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

Reduce ambiguity around RegisterForReflection's defaults #38496

Merged

Conversation

zakkak
Copy link
Contributor

@zakkak zakkak commented Jan 31, 2024

Motivated by #24474 (comment)

@humcqc
Copy link
Contributor

humcqc commented Jan 31, 2024

Hi @zakkak ,
Thanks for this clarification.
Interesting, in my case I think it's better to use the ignoreNested attribute set to false.
Are we sure the default behavior should be set to true ?
I mean all people that will have nested attributes would like to have them registered automatically or I miss some performance concerns ?

@zakkak
Copy link
Contributor Author

zakkak commented Jan 31, 2024

Changing ignoreNested's default could indeed be an acceptable trade-off. I don't expect classes to have a large number of nested classes (as a result the impact is expected to be insignificant), and for those that do there is always the option to change the default.

cc @geoand (since you originally reviewed #9182)

@zakkak
Copy link
Contributor Author

zakkak commented Jan 31, 2024

@geoand thanks for the review. Do you have any comments regarding changing the default for ignoreNested? See #38496 (comment).

@geoand
Copy link
Contributor

geoand commented Jan 31, 2024

Yeah, I think it would be fine

@zakkak
Copy link
Contributor Author

zakkak commented Jan 31, 2024

@humcqc would you like to create a PR changing the default and the corresponding documentation?

@zakkak zakkak added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 31, 2024
@humcqc
Copy link
Contributor

humcqc commented Jan 31, 2024

@humcqc would you like to create a PR changing the default and the corresponding documentation?

will do after this one: #36594

Copy link

quarkus-bot bot commented Jan 31, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit b25bb6a.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@zakkak zakkak merged commit 668284e into quarkusio:main Jan 31, 2024
49 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 31, 2024
@zakkak zakkak deleted the 2024-01-31-register-for-reflection-doc branch January 31, 2024 14:22
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Jan 31, 2024
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.

3 participants