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

ErrorAttributeOptions.excluding throws IllegalArgumentException if includes is empty #24230

Closed
wants to merge 1 commit into from

Conversation

@wanderleisouza
Copy link
Contributor

@wanderleisouza wanderleisouza commented Nov 22, 2020

No description provided.

…on when includes is empty. Fixes gh-24153
@spencergibb
Copy link
Member

@spencergibb spencergibb commented Nov 22, 2020

Did you target the wrong branch?

@snicoll snicoll changed the base branch from 2.3.x to master Nov 23, 2020
@snicoll
Copy link
Member

@snicoll snicoll commented Nov 23, 2020

@wanderleisouza thanks for the PR.

Thanks @spencergibb, wrong branch indeed. I've fixed that and we can decide where to target this when we review/polish the PR.

@wanderleisouza
Copy link
Contributor Author

@wanderleisouza wanderleisouza commented Nov 23, 2020

oh, thank you @snicoll and @spencergibb. I think it should be in 2.3.x, no? Let me know how to proceed (notice concourse-ci/status is broken due to boot:checkstyleTest). Should I submit a new PR?

@snicoll
Copy link
Member

@snicoll snicoll commented Nov 23, 2020

I think it should be in 2.3.x, no?

As I've already indicated we can merge the PR in the appropriate branches as part of reviewing the PR.

notice concourse-ci/status is broken due to boot:checkstyleTest). Should I submit a new PR?

Ideally you should have built the project locally before submitting the PR, which would have revealed the build failure. We can also take care of that as part of reviewing.

Please don't open a new PR as it'd create unnecessary noise. FYI, pushing additional changes to your existing branch will update this PR.

@snicoll snicoll changed the title Fixes ErrorAttributeOptions.excluding throwing IllegalArgumentException when includes is empty. Fixes gh-24153 ErrorAttributeOptions.excluding throws IllegalArgumentException if includes is empty Nov 23, 2020
@snicoll snicoll added this to the 2.3.x milestone Nov 23, 2020
@snicoll snicoll self-assigned this Nov 23, 2020
@snicoll snicoll modified the milestones: 2.3.x, 2.3.7 Nov 23, 2020
snicoll added a commit that referenced this pull request Nov 23, 2020
@snicoll snicoll closed this in c383ab7 Nov 23, 2020
@snicoll
Copy link
Member

@snicoll snicoll commented Nov 23, 2020

@wanderleisouza thank you for making your first contribution to Spring Boot. I've polished it and applied to both 2.3.x and master.

I've reworked the tests in particular as the assertions were not running properly. It can be tricky with assertJ but the assertion should run "outside" of the main thing to assert. So assertThat(errorAttributeOptions.getIncludes().isEmpty()); should have been assertThat(errorAttributeOptions.getIncludes()).isEmpty();. This assertion was wrong for the include use case so I've fixed that too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants