-
Notifications
You must be signed in to change notification settings - Fork 18
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
Fix nested attribute name and generated custom value method name conflicts #81
Conversation
…e when the attribute name matches any of the generated custom value method names (#76)
For additional context: I do not believe we can we get away with unexported (lowercase first letter) field names which would wholly eliminate this problem since the framework's reflection needs them to be exported. So given that, I don't think we have much choice in this situation -- we need to munge the field names in some fashion. Another option could be to potentially suffix (these or all) field names with "Attribute"/"Field" or similar. Not sure if that's any better or worse than prefixing in terms of developers trying to find field names that are now mismatched with the attribute names. |
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, one nit
// Example: | ||
// - equal(something) -> somethingEqual | ||
// - type(something) -> somethingType | ||
func (identifier FrameworkIdentifier) ToPrefixCamelCase(name string) string { |
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.
nit: Would changing the name of the input parameter to prefix
make more sense in this context? Same for the method below
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.
Fair point. Have updated.
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Closes: #76
Refer to issue comment for details regarding the changes in this PR.