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

Use Swagger schema format to distinguish wrapper types from primitives #167

Merged
merged 4 commits into from
Jul 21, 2021

Conversation

evanrelf
Copy link
Contributor

@evanrelf evanrelf commented Jul 20, 2021

Notes:

  • These format values are non-standard / incorrect Swagger, so they're hidden behind a flag (off by default).
  • Replacing the original instance ToSchema (OverrideToSchema ByteString) is okay because we don't have optional anymore, therefore Maybe ByteString now unambiguously refers to BytesValue.

cc @natefaubion

Commits are atomic.

@@ -43,6 +48,8 @@ library
exposed-modules: Proto3.Suite.DotProto.Generate.Swagger
build-depends: swagger2 >=2.1.6 && <2.7
cpp-options: -DSWAGGER
if flag(swagger-wrapper-format)
cpp-options: -DSWAGGER_WRAPPER_FORMAT
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using CPP, a cleaner approach is to move all of the code into a separate module, which is conditionally included in the .cabal file. In other words:

if flag(swagger-wrapper-format)
  hs-source-dirs: swagger

The other benefit of doing it this way is that it also lets you conditionally depend on any necessary modules (not that we need that in this particular case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah you're right, in this case it would be a lot cleaner that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm actually, I'm not so sure. I think I still need -XCPP for the conditional import. I'm inclined to stick with just -XCPP, and not -XCPP + a new module. As much as I hate -XCPP, in this case I think it's already as simple as it's going to get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is something better addressed in a future, more broad refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need a conditional import if you do it like this:

  • Decide on the name for the separate module (e.g. Proto3.Suite.DotProto.Generate.Instances)
  • Create two source directories: instances and no-instances
  • Under no-instances, place a blank module of the desired name
  • Under instances, place the module with the instances
  • Use an if expression in the .cabal file to determine which source directory to import based on the configure flag
  • Proto3.Suite.DotProto.Generate.Swagger unconditionally imports the desired module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point. I like that idea. Would you mind if I tackled that in a follow-up PR, so I can apply that technique to all our flags?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, that's fine

@evanrelf evanrelf merged commit a8a7032 into master Jul 21, 2021
@evanrelf evanrelf deleted the evan/wrapper-format branch July 21, 2021 16:53
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.

3 participants