Skip to content

Polish #14667

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

Closed
wants to merge 1 commit into from
Closed

Polish #14667

wants to merge 1 commit into from

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Oct 3, 2018

This PR fixes some typos and polishes trivial stuff.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 3, 2018
@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 3, 2018
@philwebb philwebb added this to the 2.1.x milestone Oct 3, 2018
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR. I am not sure about one change so I've added a question.

"description": "Minimum \"Content-Length\" value that is required for compression to be performed.",
"type": "org.springframework.util.unit.DataSize",
"defaultValue": "2KB"
},
Copy link
Member

Choose a reason for hiding this comment

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

Can you share a bit more context why this got removed? I am afraid I had to add it because Compression is in spring-boot and the annotation processor can't read the source description on it.

If you've missed that, you're raising a red flag anyway as we should probably document all properties consistently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snicoll Thanks for the feedback! I just expected that comments on fields in Compression will be harvested for the configuration metadata by the annotation processor. Do I have a wrong expectation?

Copy link
Member

Choose a reason for hiding this comment

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

Do I have a wrong expectation?

You don't but, unfortunately, it doesn't work that way at the moment.

Compression is in the spring-boot module and it is being used in the spring-boot-autoconfigure module. When the annotation processor runs, it doesn't have access to the source model of Compression (as it's not being compiled in that round). I can see one way to fix that by adding the sources of spring-boot when compiling spring-boot-auto-configure.

In any case, let's raise a separate issue for this and we can take it from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snicoll Thanks for the quick feedback! I reverted the change in 27b987c. I guess the other three fields need to be documented there as they are absent until the issue is resolved.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Yes, let's figure this out in #14669 (as manually documenting them may be the outcome of that issue).

@snicoll snicoll added type: task A general task status: waiting-for-feedback We need additional information before we can continue and removed type: enhancement A general enhancement labels Oct 3, 2018
@snicoll snicoll removed the status: waiting-for-feedback We need additional information before we can continue label Oct 3, 2018
@snicoll snicoll self-assigned this Oct 3, 2018
@snicoll snicoll modified the milestones: 2.1.x, 2.1.0.RC1 Oct 3, 2018
snicoll pushed a commit that referenced this pull request Oct 3, 2018
@snicoll snicoll closed this in 0eae323 Oct 3, 2018
snicoll added a commit that referenced this pull request Oct 3, 2018
* pr/14667:
  Polish contribution
  Polish
@izeye izeye deleted the polish-20180903 branch October 3, 2018 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants