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

Prevent confusion about build(boolean) method in UriComponentsBuilder #25604

Closed
wants to merge 1 commit into from
Closed

Conversation

eitan613
Copy link

To prevent confusion about the build(boolean) method

To prevent confusion about the build(boolean) method
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 17, 2020
@rstoyanchev
Copy link
Contributor

@eitan613 I'm fine to make some Javadoc improvements but I'm not sure if the proposed change makes much of a difference. Either way the encoded flag states what it means. The fact that this will be validated/asserted doesn't aid in understanding.

I was thinking rather that the UriComponentsBuilder.fromUri(URI) method should state that you must then use .build(true) and reversely on .build(true) point out that if the builder was created from a URI the flag should be true.

Or otherwise please clarify what confused you so we can find a way to improve.

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Aug 20, 2020
@eitan613
Copy link
Author

@rstoyanchev truth be told I wasn't really using this code and after looking at it a bit I wasn't really confused about it. I noticed that others were, the user who opened up issue #25444 for example, and when I looked around online it seems others were confused also. It sounds to me like the change about the UriComponentsBuilder.fromUri(URI) would help but it also seemed to me that people thought that the build(true) method was encoding the URI. In which case it might be helpful to clarify that it is not encoding and the boolean passed in is to verify if it was already encoded

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 23, 2020
@rstoyanchev
Copy link
Contributor

Okay I see. I think it already says that but we can certainly strengthen the language used. It does sound more like the confusion might be from looking at the method and assuming what it means.

@rstoyanchev rstoyanchev added this to the 5.2.9 milestone Aug 24, 2020
@rstoyanchev rstoyanchev self-assigned this Aug 24, 2020
@rstoyanchev rstoyanchev added type: documentation A documentation task type: task A general task and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels Aug 24, 2020
@eitan613
Copy link
Author

Anything else I can/should do then?

@rstoyanchev rstoyanchev added the for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x label Sep 4, 2020
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x labels Sep 4, 2020
rstoyanchev added a commit that referenced this pull request Sep 8, 2020
rstoyanchev added a commit that referenced this pull request Sep 8, 2020
rstoyanchev added a commit that referenced this pull request Sep 8, 2020
engimatic pushed a commit to engimatic/spring-framework that referenced this pull request Sep 29, 2020
zx20110729 pushed a commit to zx20110729/spring-framework that referenced this pull request Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: backported An issue that has been backported to maintenance branches type: documentation A documentation task type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants