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

Support proto3-suite 0.5.0 and later. #22

Merged
merged 5 commits into from
Jul 12, 2022
Merged

Conversation

j6carey
Copy link
Contributor

@j6carey j6carey commented Jul 7, 2022

Starting with proto3-suite 0.5.0, the initial character of
a service method names is no longer forced to uppercase.

Starting with proto3-suite 0.5.0, the initial character of
a service method names is no longer forced to uppercase.
@j6carey j6carey requested a review from Friede80 July 7, 2022 23:06
Copy link

@jcarr-awake jcarr-awake left a comment

Choose a reason for hiding this comment

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

This is clear enough for me, my I-dont-know-anything-about-the-original-change question is why we wouldn't be able to just use prefixedFieldName always, unless it didn't exist?

Copy link
Contributor

@Friede80 Friede80 left a comment

Choose a reason for hiding this comment

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

I don't quite understand the difference between the two. The -ddump-splices output looks the same. (Although that very well could be the test file doesn't exercise this difference)

Also, we use prefixFieldName in several other places as well. Wouldn't they need to be updated as well?

@j6carey
Copy link
Contributor Author

j6carey commented Jul 8, 2022

@Friede80, @jcarr-awake ,

prefixedMethodName was added by awakesecurity/proto3-suite#171 (first appearing in proto3-suite 0.5.0) and used for service method names only, with prefixedFieldName still used for other things. Apparently the desire is to make it possible for two methods of the same service to differ only in the case of the first letter of their method names.

I ran into a compilation failure in an Awake application of this library before making this change, because the Haskell spelling of at least one service method was wrong.

Regarding other uses of prefixedFieldName in grpc-mqtt: thanks for spotting that problem with this PR; I'll push up a change.

@j6carey
Copy link
Contributor Author

j6carey commented Jul 8, 2022

c346d57 replaces the other uses of prefixedFieldName and isolates the conditional code to a compatibility module.

Copy link
Contributor

@Friede80 Friede80 left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks for moving it out to a separate module.

It does raise a separate issue that the tests fail when using proto3-suite >= 0.5.0, because the generated code is using version 0.4.3. I think we are still good to merge this PR and I'll create an issue for fixing the tests.

src/Proto3/Suite/DotProto/Internal/Compat.hs Outdated Show resolved Hide resolved
Co-authored-by: Matt Friede <friede80@gmail.com>
so that when they are incorporated Haskell names they retain
the same capitalization whe built with proto3-suite-0.5.0 as
they did with earlier bersions of proto3-suite.
@j6carey
Copy link
Contributor Author

j6carey commented Jul 8, 2022

@Friede80 , I think the test failures were a legitimate concern. I see two solutions: use the old name creation function for the functions that grpc-mqtt adds that are not in the .proto file, or just capitalize their names in the grpc-mqtt source so that both old and new versions of proto3-suite will produce the same compound name. I adopted the latter solution in ab569ab .

Edited to add that it seems the test failures I saw were different than those you saw. My change was focused on the test failures that I saw.

@j6carey j6carey merged commit c511a7f into main Jul 12, 2022
@j6carey j6carey deleted the support-proto3-suite-0.5 branch July 12, 2022 15:09
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.

None yet

3 participants