Conversation
ccycle
force-pushed
the
no-field-prefix
branch
2 times, most recently
from
a7810c5 to
aebd273
Compare
ccycle
force-pushed
the
no-field-prefix
branch
3 times, most recently
from
d89c30d to
04e93ac
Compare
ccycle
force-pushed
the
no-field-prefix
branch
from
04e93ac to
9197b19
Compare
|
@riz0id Could you review this PR? |
j6carey
left a comment
j6carey
left a comment
There was a problem hiding this comment.
Thanks for this feature. Mostly this looks good, though I do have some requests.
| prefixedMethodNameWithFlag :: MonadError CompileError m => IsPrefixed -> String -> String -> m String | ||
| prefixedMethodNameWithFlag _ _ "" = invalidTypeNameError "<empty name>" | ||
| prefixedMethodNameWithFlag (IsPrefixed flag) serviceName (x : xs) | ||
| | flag = prefixedMethodName serviceName (x : xs) | ||
| | isLower x = return (fieldLikeName (x : xs)) | ||
| | otherwise = fieldLikeName <$> typeLikeName (x : xs) | ||
|
|
There was a problem hiding this comment.
Do you need a keyword check in this function too?
There was a problem hiding this comment.
This is needed for the same reason in this comment (#228 (comment)), since the function is used in generating service field name and needed to avoid parse error.
There was a problem hiding this comment.
OK, so if it is needed, then could you add it?
There was a problem hiding this comment.
Thank you for your comment, I added a commit including changes you requested. Could you please review for that?
There was a problem hiding this comment.
Does prefixedMethodNameWithFlag also need a keyword check?
There was a problem hiding this comment.
@j6carey Sorry I forgot including the commit, I'll add it.
@riz0id Thank you for the review. I'm not sure I follow your opinion correctly, but I also agree adding hs_name option and removing the part of my changes to avoid colliding with keywords.
I strongly suggest not trying to do any case convention changes and just use the name of service methods and message fields that are provided in the protobuf source. I can't think of any situations where there is a benefit to modifying the lexical content names. Also, theres no function that exists that can convert snake case to camel case without opening up the potential for unintended name shadowing in the code generator.
I have a question about this part. In this PR, should the use of toCamelCase be avoided, or allowed temporally to consider that it will be removed in the future?
There was a problem hiding this comment.
In this PR and until we have a hs_name option, we should stick to the naming convention rules that the rest of proto3-suite uses for record selector names which is what you do here with toCamelCase. In the future though, when we have facilities to override names in generated Haskell code, we will probably make breaking changes to instead use snake case for records. Currently the number of edge cases that need to be considered in order to rename protobuf names to camel case Haskell identifiers is getting out of hand. Using snake case record selectors makes a lot more sense (imo) since:
- Protobuf suggests snake case for message field names by convention.
- Snake case would not require specific rules to avoid collisions with keywords or double underscores (i.e. "__").
- Many people (myself included) already use snake case by convention to distinguish record selectors from ordinary functions.
tldr; Yes use toCamelCase for now, but once proto3-suite supports a hs_name option expect breaking changes that use snake case for message field names.
There was a problem hiding this comment.
My concerns have been addressed, but @riz0id , did you want the keyword checks eliminated, or are they OK until hs_name is available?
tools/compile-proto-file/Main.hs
Outdated
|
|
||
| isPrefixed = IsPrefixed . not <$> switch ( | ||
| long "no-field-prefix" | ||
| <> help "Remove prefix for record field names" |
There was a problem hiding this comment.
This is a nitpicking, but perhaps more natural explanation of the behaviour would be "do not prefix record field names by the type names", since it does not remove anything when this is enabled.
There was a problem hiding this comment.
Thank you, I fixed this part of the message.
4 participants
Added option to remove field name prefixes from records generated by
compile-proto-file. This change is intended for use withNoFieldSelectorsfor auto-generated records.