Skip to content

Polish #13963

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 #13963

wants to merge 1 commit into from

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Jul 31, 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 Jul 31, 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 item, can you please have a look?

@@ -412,8 +412,8 @@ public void process(AnnotationMetadata annotationMetadata,
DeferredImportSelector deferredImportSelector) {
Assert.state(
deferredImportSelector instanceof AutoConfigurationImportSelector,
String.format(
"AutoConfigurationImportSelector only supports %s implementations, got %s",
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a typo but a conscious decision. AutoConfigurationGroup is an internal concept while the one in the exception is the public concept.

What do you think?

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 clarification! When I saw it, "AutoConfigurationImportSelector only supports AutoConfigurationImportSelector implementations" sounds a bit weird and it's in AutoConfigurationGroup.process(), so I was trying to change it to AutoConfigurationGroup which sounds more natural but noticed that the class is private scope as you mentioned. I wasn't sure with this change at that time but with your comment, it'd be better to be reverted for now. I'll revert it soon.

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 I reverted it and force-pushed.

Copy link
Member

Choose a reason for hiding this comment

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

You got a valid point in any case. I'll reword that exception message

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Aug 1, 2018
@izeye izeye force-pushed the polish-20180801 branch from 20aef61 to fb9a724 Compare August 1, 2018 08:41
@snicoll snicoll self-assigned this Aug 1, 2018
@snicoll snicoll added type: task A general task for: merge-with-amendments Needs some changes when we merge and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Aug 1, 2018
snicoll pushed a commit that referenced this pull request Aug 1, 2018
snicoll added a commit that referenced this pull request Aug 1, 2018
@snicoll snicoll added this to the 2.0.5 milestone Aug 1, 2018
snicoll pushed a commit that referenced this pull request Aug 1, 2018
@snicoll snicoll closed this in f7032bd Aug 1, 2018
snicoll added a commit that referenced this pull request Aug 1, 2018
* pr/13963:
  Polish contribution
  Polish
@snicoll
Copy link
Member

snicoll commented Aug 1, 2018

Thanks again @izeye - I've hopefully improved the message in f7032bd

@izeye izeye deleted the polish-20180801 branch August 1, 2018 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants