-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat: adds the ability to change the default tag name (#27) #28
Conversation
9812e96
to
2b5072f
Compare
Signed-off-by: hohmannr <[email protected]>
2b5072f
to
b102547
Compare
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
==========================================
+ Coverage 88.95% 89.04% +0.09%
==========================================
Files 12 12
Lines 1747 1762 +15
==========================================
+ Hits 1554 1569 +15
Misses 138 138
Partials 55 55
☔ View full report in Codecov by Sentry. |
The idea is great, but not sure how you will handle the maintainability and readability of your application later. |
That is a very good question. We have solved this issue with another package, here goes the story: We have lightweight wrappers around multiple packages, which set them up to our needs. E.g. configuring them in an type UserInfo struct {
FirstName string `validate_gateway:"required,max=256" validate_api:"required,max=256"`
LastName string `validate_gateway:"required,max=256" validate_api:"required,max=256"`
Source string `validate_gateway:"eq=gateway" validate_api:"eq=api"`
Timestamp string `validate_gateway:"_rfc3339" validate_api:"_rfc3339"`
} The same strategy can be used for type UserInfo struct {
FirstName string `faker_gateway:"first_name" faker_api:"first_name"`
LastName string `faker_gateway:"last_name" faker_api:"last_name"`
Source string `faker_gateway:"_source_gateway" faker_api:"_source_api"`
Timestamp string `faker_gateway:"_rfc3339" faker_api:"_rfc3339"`
} |
pkg/errors/generic.go
Outdated
@@ -16,6 +16,7 @@ var ( | |||
ErrTagNotSupported = "Tag unsupported: %s" | |||
ErrTagAlreadyExists = "Tag exists" | |||
ErrTagDoesNotExist = "Tag does not exist" | |||
ErrTagNameInvalid = "Tag name is invalid" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this error tho.
It brings confusion with the existing error message related to tag -- see other error related to tag
Can we renamed it with something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious how validator library decide their error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They do not check for valid tagnames^^ https://github.com/go-playground/validator/blob/bd1113d5c1901c5205fc0a7af031a11268ff13ee/validator_instance.go#L154
What I would suppose and what I have seen somewhere else (do not remember where anymore), they called the tag name field tag identifier. This would make for more succinct error names.
ErrFieldTagIdentifierInvalid = "Field tag identifier invalid"
Apart from this, one could make it clear for the user of the package, what went wrong, by copying the approach of ErrTagNotSupported
and use ErrFieldTagIdentifierInvalid = "Field tag identifier invalid: %s"
in conjunction with fmt.Errorf(ErrFieldTagIdentifierInvalid, "<bad-tag-identifier>")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ErrFieldTagIdentifierInvalid LGTM, can u help to update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pkg/options/options.go
Outdated
// WithTagName sets the tag name to use. Default tag name is 'faker'. | ||
func WithTagName(tagName string) OptionFunc { | ||
if tagName == "" { | ||
err := errors.New(fakerErrors.ErrTagDoesNotExist) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error may bring confusion, is it they tag key, or the tag name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right. Do not know what I was thinking. Should be the new error that was introduced in pkg/errors/generic.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help to fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Hey @bxcodec! Hope you are fine! Just wanted to quickly check the status on this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hi @hohmannr sorry for the late response, was moving out busy IRL.
Can u help to test it first from the master branch after merged?
If no issues, I'll release new version
This would solve the problem described in #27.
I have added the ability to change the default tag name to a custom one, which is configurable via an
options.OptionFunc
. I tried to be minimal invasive and have added a test to cover the custom tag case and an example file, so people can find this feature in go.pkg reference and the github repo.