Skip to content

Remove unnecessary throws declaration in tests #26441

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 2 commits into from

Conversation

weixsun
Copy link
Contributor

@weixsun weixsun commented May 12, 2021

This pr is only used to check the test method.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 12, 2021
@@ -80,7 +79,7 @@ void close() {
}

@Test
void dataSourceIsAvailableFromJndi() throws IllegalStateException, NamingException {
void dataSourceIsAvailableFromJndi() throws IllegalStateException {
Copy link
Member

Choose a reason for hiding this comment

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

IllegalStateException is unchecked so it doesn't need to be declared either. It'd be nice to get rid of those too if you have the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, let me do it, I think there may be many similar problems in this project, I will check them one by one in the next two days, please do not merge too early

Copy link
Member

Choose a reason for hiding this comment

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

Thanks very much. I'll mark this as a draft for now so that we know it's not quite ready to be merged.

@weixsun weixsun changed the title Exception 'java.lang.Exception' is never thrown in the test method Exception is never thrown in the test method May 12, 2021
@wilkinsona wilkinsona marked this pull request as draft May 12, 2021 17:20
@weixsun weixsun force-pushed the common-checkstyle branch from 469de88 to 4d24f60 Compare May 13, 2021 08:34
@weixsun weixsun marked this pull request as ready for review May 13, 2021 10:00
@weixsun
Copy link
Contributor Author

weixsun commented May 13, 2021

@wilkinsona I have checked all the test cases and erased them. You can take time to review them again. In addition, I created a new branch based on the older main branch, so there is a file MetricsHealthMicrometerExportTests conflict, can you help me deal with it?

Copy link

@cuspymd cuspymd left a comment

Choose a reason for hiding this comment

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

There are many files whose year in the license header has not been updated. Is it okay to not update it?

@weixsun
Copy link
Contributor Author

weixsun commented May 13, 2021

There are many files whose year in the license header has not been updated. Is it okay to not update it?

😅 I forgot about it. It must be updated, let me make a quick change

@philwebb
Copy link
Member

@cuspymd For future reference, we don't require pull-requests to be 100% perfect and it's fine if things like copyright headers aren't updated. We don't want to put people off contributing. We'll generally fix that kind of thing up during the merge.

@philwebb philwebb added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels May 13, 2021
@philwebb philwebb added this to the 2.5.x milestone May 13, 2021
@cuspymd
Copy link

cuspymd commented May 14, 2021

@philwebb Thanks for responding. I also find it very inefficient to change the license header. It seems that policies related to this are different for each spring-project. For example spring-framework guides license update in coding style guide. There seems to be no explicit guide for contribution or coding style in spring-boot.

@snicoll
Copy link
Member

snicoll commented May 17, 2021

@cuspymd the coding style guide in spring framework is mostly intended to the team. We don't expect contributors to read and apply all these rules. As Phil mentioned, we're happy to fine tune formatting and conventions and don't expect PRs to be 100% perfect. In short, the review you did above isn't necessary, thank you.

@snicoll snicoll changed the title Exception is never thrown in the test method Remove unnecessary throws declaration in test methods May 17, 2021
@snicoll snicoll self-assigned this May 17, 2021
@snicoll snicoll modified the milestones: 2.5.x, 2.4.x, 2.5.0 May 17, 2021
@snicoll snicoll changed the title Remove unnecessary throws declaration in test methods Remove unnecessary throws declaration in tests May 17, 2021
snicoll pushed a commit that referenced this pull request May 17, 2021
@snicoll snicoll closed this in 21a3f03 May 17, 2021
@snicoll
Copy link
Member

snicoll commented May 17, 2021

Thanks again @weixsun

@weixsun weixsun deleted the common-checkstyle branch May 17, 2021 08:27
@cuspymd
Copy link

cuspymd commented May 17, 2021

@snicoll Thanks for the explanation. In the future, I will not review the coding style.

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.

6 participants