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.

Sign up for GitHub

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

Jump to bottom

Hyphenate @RestHeader sourceName by default #16003

Merged
merged 1 commit into from
Mar 30, 2021
Merged

Hyphenate @RestHeader sourceName by default #16003

merged 1 commit into from
Mar 30, 2021

Conversation

jjaferson
Copy link
Contributor

@jjaferson jjaferson commented Mar 24, 2021 •
edited by gastaldi
Loading

Hyphenate @RestHeader sourceName by default

Resolves #13665

@jjaferson jjaferson force-pushed the rest-header-hyphenate branch 2 times, most recently from 46f82d8 to 6bda1d2 Compare March 25, 2021 11:28
@jjaferson jjaferson force-pushed the rest-header-hyphenate branch from 6bda1d2 to 7511fde Compare March 25, 2021 16:37
gastaldi
gastaldi approved these changes Mar 25, 2021
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

LGTM

@gastaldi gastaldi requested a review from geoand March 25, 2021 17:01
@gastaldi
Copy link
Contributor

I'll let @geoand have the final say on this PR

@geoand
Copy link
Contributor

geoand commented Mar 25, 2021

Thanks for this!

I have a question though: what happens if the user actually sets a value in the @RestParam annotation?

@gastaldi
Copy link
Contributor

I have a question though: what happens if the user actually sets a value in the @RestParam annotation?

According to https://github.com/quarkusio/quarkus/pull/16003/files#diff-81713f62adc649d447ab44049899a4edd826be9be784b1aa3b654b4a22febf44R825 I think it will use what's defined there.

@jjaferson can we avoid calling the StringUtil.hyphenate when a value is set? That operation seems costly and unnecessary in this case

@jjaferson
Copy link
Contributor Author

e avoid calling the StringUtil.hyphenate when a value i

Good catch, that makes seems, I added a check to verify if the rest param is set before hyphenating it.

@jjaferson jjaferson force-pushed the rest-header-hyphenate branch from 7511fde to 3442127 Compare March 26, 2021 17:46
@jjaferson
Copy link
Contributor Author

Thanks for this!

I have a question though: what happens if the user actually sets a value in the @RestParam annotation?

no problem, that would be hyphenated again but now I added a check to verify if the restParam was set.

@geoand
Copy link
Contributor

geoand commented Mar 26, 2021

Thanks for this!
I have a question though: what happens if the user actually sets a value in the @RestParam annotation?

no problem, that would be hyphenated again but now I added a check to verify if the restParam was set.

We would also need a test for this behavior as well

@jjaferson jjaferson force-pushed the rest-header-hyphenate branch from 3442127 to 71b0595 Compare March 29, 2021 11:51
@jjaferson
Copy link
Contributor Author

Thanks for this!
I have a question though: what happens if the user actually sets a value in the @RestParam annotation?

no problem, that would be hyphenated again but now I added a check to verify if the restParam was set.

We would also need a test for this behavior as well

added tests for all the cases - when the @restParam isn't specified and when it's empty

geoand
geoand approved these changes Mar 29, 2021
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Thanks!

@gastaldi gastaldi added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Mar 29, 2021
@geoand geoand merged commit 9650c32 into quarkusio:main Mar 30, 2021
@quarkus-bot quarkus-bot bot added this to the 1.14 - main milestone Mar 30, 2021
@FroMage
Copy link
Member

FroMage commented Mar 30, 2021

Huh, I was meaning to review this PR, but never got notified, so I didn't know it was filed. Don't we get /CC on PRs?

@FroMage
Copy link
Member

FroMage commented Mar 30, 2021

I mean, your reviews were good, I wouldn't have done it differently, but I'm surprised I didn't get notified.

@geoand
Copy link
Contributor

geoand commented Mar 30, 2021

There is no notification from the bot on PRs unfortunately

@jjaferson
Copy link
Contributor Author

Huh, I was meaning to review this PR, but never got notified, so I didn't know it was filed. Don't we get /CC on PRs?

Sorry @FroMage I should have pinged you on the PR, I thought you would be notified when I added the reference to the issue you created...

@FroMage
Copy link
Member

FroMage commented Mar 30, 2021

There is no notification from the bot on PRs unfortunately

Is this on purpose, or due to a limitation, or just that we're missing a bot action?

@geoand
Copy link
Contributor

geoand commented Mar 30, 2021

Likely the latter.

Something maybe that @SaumyaSingh1 can work on if @gsmet agrees?

@saumya1singh
Copy link
Contributor

Sure :)

@FroMage
Copy link
Member

FroMage commented Mar 30, 2021

Yeah, great idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastaldi gastaldi gastaldi approved these changes

@geoand geoand geoand approved these changes

Assignees
No one assigned
Labels
area/rest triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
2.0.0.Alpha1
Development

Successfully merging this pull request may close these issues.

RESTEasy Reactive: change default name mapping strategy for @RestHeader
5 participants
@jjaferson @gastaldi @geoand @FroMage @saumya1singh