Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding nested tags for slice of strings #49

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

HARDY8118
Copy link

As pointed out in the issue #37 (faker slice of urls don't work), struct fields do not support faker tags for the underlying field data type in case of slices. Taking a subsection of the mentioned issue

type Entity struct {
    ...
    Uris []string        `faker:"slice_len=3, url"`  // makes all structure empty. I just need slice with 3 urls
    ...
}

In the above code taken from issue #37 the Uris slice will not take into account the url tag.

For the mentioned issue, I suggest an extension to the faker struct tags syntax to allow nested tags with following syntax:

type SomeType struct {
    ...
    SomeSliceOfStrings []string `faker:"<tags for slice>, [<tags for string>]"`
    ...
}
// Example
type Emails struct {
    EmailList []string `faker:"slice_len=3, [email]"`
}

The PR is still a work in progress but I would like to hear more and discuss on the idea of using nested tags.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 76.92308% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 88.97%. Comparing base (6d0a05f) to head (7a75357).

Files Patch % Lines
faker.go 76.92% 4 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #49      +/-   ##
==========================================
- Coverage   89.01%   88.97%   -0.05%     
==========================================
  Files          13       13              
  Lines        1812     1832      +20     
==========================================
+ Hits         1613     1630      +17     
- Misses        142      144       +2     
- Partials       57       58       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bxcodec
Copy link
Contributor

bxcodec commented Mar 8, 2024

Hi @HARDY8118

Thanks for the idea. I don't see any harm having the nested (yet).
Maybe can you add more usecases/tests?
Worried if we add this, then people will expecting more, want to see what are the limitation for this approach

And wondering from user experience, will this okay? As you can see from the issue, people expect it differently 🤔

@HARDY8118
Copy link
Author

Hi @bxcodec

As I mentioned initially in the PR, the idea is still a work in progress, I created this PR to get some initial thoughts on the overall idea of having such feature. As you can notice from the code change, currently I have implemented the changes just for slice of strings just to get some comments on the idea and have a discussion about it (while we are discussing on this PR I am drafting some more changes and adding tests for more use cases).

From user experience standpoint I think this is am okayish approach as it clearly defines what the user expects in both the contexts, first being from the context of slice (properties like slice_len) and other being from the context of the underlying data which the slice consists of (like in this case language and type of string). This will help moving forward if there exists some overlapping properties providing clarity and control over how each part behaves.

However I would love to hear from you and others as well on what can be done better for improving the user experience.

@bxcodec
Copy link
Contributor

bxcodec commented Jun 17, 2024

@rubemlrm any opinion from your side?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants