Skip to content

Pass mapstructSpi.enumPostfix as compilerArg#133

Merged
seime merged 6 commits intoentur:developfrom
ro0sterjam:pass-enum-postfix-as-compiler-arg
Nov 3, 2021
Merged

Pass mapstructSpi.enumPostfix as compilerArg#133
seime merged 6 commits intoentur:developfrom
ro0sterjam:pass-enum-postfix-as-compiler-arg

Conversation

@ro0sterjam
Copy link
Contributor

@ro0sterjam ro0sterjam commented Oct 26, 2021

Allow configuration of enumPostfix by means of a compilerArg.

e.g.

<compilerArgs>
    <arg>-AmapstructSpi.enumPostfixOverrides=com.package.root.a=POSTFIX_1,com.package.root.b=POSTFIX_2</arg>
</compilerArgs>

Unfortunately I was unable to find a "clean" way to pass the ProcessingEnvironment options from MapStruct's MappingProcessor so I had to create my own to pass the options to a static variable (terrible, I know). Please let me know if you have any better suggestions.

This should be safe because the processors are all initialized together first, before calling process thus the OPTIONS static field should be initialized by the time it is called on by ProtobufEnumMappingStrategy.

@ro0sterjam ro0sterjam force-pushed the pass-enum-postfix-as-compiler-arg branch 2 times, most recently from 648604a to 5ab5e44 Compare October 26, 2021 16:08
@ro0sterjam ro0sterjam force-pushed the pass-enum-postfix-as-compiler-arg branch from 61ec81c to bb7c753 Compare October 26, 2021 21:53
Copy link
Contributor

@seime seime left a comment

Choose a reason for hiding this comment

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

I was thinking that this should be made configurable pr package since one may one package with proto files using one style, and another using a different style.

WDYT?

@ro0sterjam
Copy link
Contributor Author

ro0sterjam commented Oct 27, 2021

How would you envision that it would work ideally? As in, how would you want to pass in the different configuration per package?

e.g.

<compilerArgs>
    <arg>-AmapstructSpi.enumPostfix=com.company.package1=UNSPECIFIED,com.company.package2=INVALID</arg>
</compilerArgs>

Or something different?

In this case, if we wanted to support non specificity of packages (in the scenarios where all protos need to be treated the same) we might also need to support wildcards...

But my opinion is that your project is more likely than not going to have some sort of consistency (either you are following Google's style guide, or Uber's, or your own) and the need for a multi-configuration is likely unnecessary for the majority of use cases. I know for us it definitely is.

@ro0sterjam
Copy link
Contributor Author

My preference is to merge as is and incrementally add more support for configurability as the need arises. In my honest opinion, an organization/individual service should be consistent with their naming conventions.

@seime
Copy link
Contributor

seime commented Oct 28, 2021

I think we should default to Google style and use this new parameter to override this for certain package roots?

@ro0sterjam
Copy link
Contributor Author

ro0sterjam commented Oct 28, 2021

I think we should default to Google style and use this new parameter to override this for certain package roots?

So how it's written right now, it will default to the Google style if the param is not passed.

Are you suggesting that the param (if supplied) should only be applied to specific specified package roots? As in, another compiler arg? If so, I was just wondering what use case you are thinking of where one would need two separate postfixes in the same project. Is it a scenario in which one is importing proto messages from multiple sources, where one source might be using the Google style and another source is using some other style?

@ro0sterjam
Copy link
Contributor Author

Updated to allow for package specific enum postfix overrides.

@ro0sterjam ro0sterjam requested a review from seime November 1, 2021 13:26
@seime seime merged commit 4d0b790 into entur:develop Nov 3, 2021
@seime
Copy link
Contributor

seime commented Nov 3, 2021

Thanks for your contribution @ro0sterjam !

ro0sterjam added a commit to ro0sterjam/mapstruct-spi-protobuf that referenced this pull request Oct 26, 2022
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.

2 participants