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

fix: change to prevent capitalization of prefixed service method names #171

Merged
merged 2 commits into from Aug 6, 2021

Conversation

riz0id
Copy link
Collaborator

@riz0id riz0id commented Aug 6, 2021

This PR changes the casing scheme applied to service methods in the code generator. Previously the code generator would take a protobuf service declaration such as

service MyService {
  rpc DubiousMethodName(string) returns (string);
  rpc dubiousMethodName(bool) returns (int);
}

and emit the following record

data MyService = MyService 
  { myServiceDubiousMethodName :: String -> String 
  , myServiceDubiousMethodName :: Bool -> Int -- Name collision 
  } 

The code generator no longer applies a casing-transformation on the first character of a service method name, this includes spans of capital letters prefixing the string. i.e. the code generator now emits myServiceDubiousMethodName and myServicedubiousMethodName. This change also includes documentation for the name resolution functions used in the code generator.

Issue #170 documents other instances where malformed datatype declarations can be emitted by the code generator, this is an incremental step in improving how the edges cases listed in that issue are handled.

Fixes:
* Changes prefixed service method casing such that the method name is camel cased rather than pascal cased.
* Fixes issue where methods different only by capitalization of first letter produce name-colliding record selectors.
@riz0id riz0id added the bug label Aug 6, 2021
Copy link
Contributor

@evanrelf evanrelf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the @since annotations you've added are incorrect (e.g. 0.1.0.0). We don't use @since anywhere else in the codebase, so I would lean towards omitting them.

Or if you really want them, we should add them everywhere in a separate PR.

@riz0id
Copy link
Collaborator Author

riz0id commented Aug 6, 2021

Some of the @since annotations you've added are incorrect (e.g. 0.1.0.0). We don't use @since anywhere else in the codebase, so I would lean towards omitting them.

Or if you really want them, we should add them everywhere in a separate PR.

I think the since annotations are a useful bit of documentation because it helps readers understand where changes begin and end. I've also found them helpful for setting package bounds when breaking changes are incorporated into a library. I understand that people might object to adding them though since some consider them annoying to maintain. If everyone who actively maintains proto3-suite is fine with me adding them though I will do so in a separate PR.

@evanrelf
Copy link
Contributor

evanrelf commented Aug 6, 2021

@riz0id I agree that they are useful, and I wouldn't mind at all if you added them. And I don't think you need buy in from every maintainer; if you're willing to do the legwork, I say go for it.

I personally find them slightly more trouble than they're worth, but my projects are usually very mercurial. I imagine there'd be a lot less friction in a more stable, slow-moving project.

@evanrelf
Copy link
Contributor

evanrelf commented Aug 6, 2021

I don't think proto3-suite or grpc-haskell have very principled releases processes or stability guarantees, so @since annotations could be useful in improving the situation.

@riz0id
Copy link
Collaborator Author

riz0id commented Aug 6, 2021

@evanrelf sounds good, I'll have a PR incorporating them sometime soon then.

@riz0id riz0id merged commit 16efe2e into master Aug 6, 2021
@riz0id riz0id deleted the jake-leach/service-name-resolution branch August 6, 2021 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants