Skip to content

Simplify MediaTypeRequestMatcher construction#6648

Closed
clevertension wants to merge 1 commit intospring-projects:masterfrom
clevertension:gh-6612
Closed

Simplify MediaTypeRequestMatcher construction#6648
clevertension wants to merge 1 commit intospring-projects:masterfrom
clevertension:gh-6612

Conversation

@clevertension
Copy link
Contributor

@clevertension clevertension commented Mar 26, 2019

this PR will simplify MediaTypeRequestMatcher construction, just like MediaTypeServerWebExchangeMatcher

see #6612

jzheaux
jzheaux requested changes Mar 26, 2019
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @clevertension! I've left some inline feedback.

@clevertension
Copy link
Contributor Author

clevertension commented Mar 27, 2019

@jzheaux it's done

jzheaux
jzheaux requested changes Mar 29, 2019
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux 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, @clevertension! I've left just a bit more feedback inline.

Also, once you have that ready, will you please squash your commits so that there is only one in the PR? Thanks!

...rc/test/java/org/springframework/security/web/util/matcher/MediaTypeRequestMatcherTests.java Outdated
Copy link
Contributor

@jzheaux jzheaux Mar 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Will you please add the appropriate line break here?

Copy link
Contributor Author

@clevertension clevertension Mar 29, 2019

Choose a reason for hiding this comment

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

ok

...rc/test/java/org/springframework/security/web/util/matcher/MediaTypeRequestMatcherTests.java Outdated
Copy link
Contributor

@jzheaux jzheaux Mar 29, 2019

Choose a reason for hiding this comment

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

A number of these tests don't actually throw an HttpMediaTypeNotAcceptableException. Will you please remove the throws from the signature where it is unnecessary? This way, the expectations of the test are clearer.

Copy link
Contributor Author

@clevertension clevertension Mar 29, 2019

Choose a reason for hiding this comment

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

ok

@jzheaux jzheaux self-assigned this Mar 29, 2019
@jzheaux jzheaux added status: duplicate A duplicate of another issue in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement labels Mar 29, 2019
@jzheaux jzheaux added this to the 5.2.0.M2 milestone Mar 29, 2019
@jzheaux
Copy link
Contributor

jzheaux commented Mar 29, 2019

Thanks again, @clevertension! This is now merged into master via a9a86cd. Note that I just added a "fixes" note to the commit, like the one here.

@jzheaux jzheaux closed this Mar 29, 2019
@clevertension
Copy link
Contributor Author

clevertension commented Mar 29, 2019

@jzheaux thanks very much for reviewing my code, i hope i can contribute more code 😃😃

@jzheaux
Copy link
Contributor

jzheaux commented Mar 29, 2019

I'd love that, @clevertension! Make sure to keep an eye out especially for tickets tagged with "help wanted" as those are usually ideal for community contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jzheaux jzheaux jzheaux approved these changes

Assignees

@jzheaux jzheaux

Labels

in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: enhancement A general enhancement

Projects

None yet

Milestone

5.2.0.M2

Development

Successfully merging this pull request may close these issues.

2 participants

@clevertension @jzheaux