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

Change audience from underscore to dash #898

Merged
merged 3 commits into from Jun 28, 2018
Merged

Change audience from underscore to dash #898

merged 3 commits into from Jun 28, 2018

Conversation

@rcillo
Copy link
Member

@rcillo rcillo commented Jun 26, 2018

rcillo added 2 commits Jun 26, 2018
It would be less harmful to keep consistency with the guidelines than
with the existing API from Nakadi.
@v-stepanov
Copy link
Contributor

@v-stepanov v-stepanov commented Jun 26, 2018

@rcillo Hm... Have you tried something like here: https://stackoverflow.com/questions/12468764/jackson-enum-serializing-and-deserializer
I think it will look nicer like that

@codecov-io
Copy link

@codecov-io codecov-io commented Jun 26, 2018

Codecov Report

Merging #898 into master will decrease coverage by 0.01%.
The diff coverage is 51.85%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #898      +/-   ##
============================================
- Coverage     53.97%   53.95%   -0.02%     
- Complexity     1759     1761       +2     
============================================
  Files           316      316              
  Lines          9692     9712      +20     
  Branches        888      892       +4     
============================================
+ Hits           5231     5240       +9     
- Misses         4135     4144       +9     
- Partials        326      328       +2
Impacted Files Coverage Δ Complexity Δ
...ain/java/org/zalando/nakadi/config/JsonConfig.java 78.94% <42.85%> (-21.06%) 3 <0> (ø)
.../main/java/org/zalando/nakadi/domain/Audience.java 64.28% <61.53%> (-35.72%) 2 <1> (+1)
...n/java/org/zalando/nakadi/service/EventStream.java 75% <0%> (+1.61%) 30% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca53711...66eb25f. Read the comment docs.

@rcillo
Copy link
Member Author

@rcillo rcillo commented Jun 27, 2018

@v-stepanov I can try. I had problem because there is a modifier registered for all enums and it somehow conflicted with other approaches. But I did not try the one you pointed out yet.

@rcillo
Copy link
Member Author

@rcillo rcillo commented Jun 28, 2018

@v-stepanov I tried the proposed alternative and it does not work because in our project we have a global override on how enums are handled. This overrides everything else. In order to use this cleaner approach, I would have to explicitly state for every enum in the project how it should be serialized and deserialized. I think in the end it would not pay off.

@@ -35,7 +36,7 @@
@Primary
public ObjectMapper jacksonObjectMapper() {
final ObjectMapper objectMapper = new ObjectMapper()
.setPropertyNamingStrategy(CAMEL_CASE_TO_LOWER_CASE_WITH_UNDERSCORES);
.setPropertyNamingStrategy(SNAKE_CASE);

This comment has been minimized.

@v-stepanov

v-stepanov Jun 28, 2018
Contributor

will not it break anything from existing behavior?

This comment has been minimized.

@rcillo

rcillo Jun 28, 2018
Author Member

I modified it because the old constant has been deprecated and replaced by this new one. They mean the same thing.

@v-stepanov
Copy link
Contributor

@v-stepanov v-stepanov commented Jun 28, 2018

@rcillo ok, thanks for checking. Then let's keep it as it is currently implemented.

@v-stepanov
Copy link
Contributor

@v-stepanov v-stepanov commented Jun 28, 2018

👍

1 similar comment
@rcillo
Copy link
Member Author

@rcillo rcillo commented Jun 28, 2018

👍

@rcillo
Copy link
Member Author

@rcillo rcillo commented Jun 28, 2018

deploy validation please

Following the API guidelines, we're implementing a restrictive
deserialization strategy https://opensource.zalando.com/restful-api-guidelines/#109
@v-stepanov
Copy link
Contributor

@v-stepanov v-stepanov commented Jun 28, 2018

👍

1 similar comment
@rcillo
Copy link
Member Author

@rcillo rcillo commented Jun 28, 2018

👍

@rcillo
Copy link
Member Author

@rcillo rcillo commented Jun 28, 2018

deploy validation please

@rcillo rcillo merged commit bbfb446 into master Jun 28, 2018
6 of 8 checks passed
6 of 8 checks passed
codecov/patch 51.85% of diff hit (target 53.97%)
Details
codecov/project 53.95% (-0.02%) compared to ca53711
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
aruha/jenkins Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
zappr Approvals: @v-stepanov, @rcillo.
zappr/pr/specification PR has passed specification checks
@rcillo rcillo deleted the aruha-1528-audience branch Jun 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.