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 module renaming in compile-proto-file #183

Merged
merged 7 commits into from
Mar 11, 2022
Merged

Conversation

riz0id
Copy link
Collaborator

@riz0id riz0id commented Mar 1, 2022

This PR changes the way compile-proto-file handles generating valid Haskell module names from protobuf files, fixes issue #182.

@riz0id riz0id linked an issue Mar 1, 2022 that may be closed by this pull request
@riz0id riz0id self-assigned this Mar 1, 2022
@j6carey j6carey self-requested a review March 1, 2022 17:32
Copy link
Collaborator

@j6carey j6carey left a comment

Choose a reason for hiding this comment

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

Overall this looks good, but in addition to the few very minor issues mentioned below, could you please increase the version number to 0.5.0 because this is a breaking change? Users will have to modify their source code to expect the new module names.

src/Proto3/Suite/DotProto/Generate.hs Outdated Show resolved Hide resolved
src/Proto3/Suite/DotProto/Generate.hs Show resolved Hide resolved
src/Proto3/Suite/DotProto/Internal.hs Show resolved Hide resolved
src/Proto3/Suite/DotProto/Generate.hs Outdated Show resolved Hide resolved
Changes:
1. remove `-XBangPatterns` in `Proto3.Suite.DotProto.Generate`
2. bump package version `0.4.3` -> `0.5.0`
Copy link
Collaborator

@j6carey j6carey left a comment

Choose a reason for hiding this comment

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

Thanks for the change, and for making those edits.

@riz0id
Copy link
Collaborator Author

riz0id commented Mar 10, 2022

I added a unit test making sure that *.proto filenames get resolved correctly. I will merge these changes into master after CI finishes.

Testing code is overkill for what it's currently used for, but will make adding tests for other renaming functions in the future much easier.

tests/Test/Proto/Generate/Name.hs Outdated Show resolved Hide resolved
tests/Test/Proto/Generate/Name/Gen.hs Outdated Show resolved Hide resolved
@riz0id
Copy link
Collaborator Author

riz0id commented Mar 11, 2022

I got frustrated with trying to handle all of the edge cases (e.g. "A_") so I just rewrote renameProtoFile with parsec.

Copy link
Collaborator

@j6carey j6carey left a comment

Choose a reason for hiding this comment

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

Thanks!

@riz0id riz0id merged commit 0c9820f into master Mar 11, 2022
@riz0id riz0id deleted the jake/fix-module-name-casing branch March 11, 2022 19:07
fumieval pushed a commit to herp-inc/proto3-suite that referenced this pull request Jun 13, 2022
* preserve character casing in module names
* bump package version `0.4.3` -> `0.5.0`
fumieval pushed a commit to herp-inc/proto3-suite that referenced this pull request Jun 13, 2022
* preserve character casing in module names
* bump package version `0.4.3` -> `0.5.0`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preserve --out directory casing in compile-proto-file
3 participants