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

do not raise on offers that do not match the media type ABNF #372

Merged
merged 6 commits into from Sep 6, 2018

Conversation

@mmerickel
Copy link
Member

@mmerickel mmerickel commented Sep 3, 2018

fixes #365

Copy link
Member

@stevepiercy stevepiercy left a comment

Optional suggestion for changelog only.

CHANGES.txt Outdated Show resolved Hide resolved
@mmerickel mmerickel force-pushed the mmerickel:ignore-invalid-offers branch from 3a268b1 to 47aa53c Sep 4, 2018
@mmerickel mmerickel force-pushed the mmerickel:ignore-invalid-offers branch from 47aa53c to a68637d Sep 4, 2018
CHANGES.txt Outdated Show resolved Hide resolved
src/webob/acceptparse.py Outdated Show resolved Hide resolved
src/webob/acceptparse.py Outdated Show resolved Hide resolved
src/webob/acceptparse.py Outdated Show resolved Hide resolved
mmerickel added 2 commits Sep 6, 2018
src/webob/acceptparse.py Outdated Show resolved Hide resolved
src/webob/acceptparse.py Show resolved Hide resolved
src/webob/acceptparse.py Show resolved Hide resolved
@bertjwregeer bertjwregeer merged commit 00c66f7 into Pylons:master Sep 6, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
AcceptNoHeader(),
['text/html', 'text/plain;p=1;q=0.5;e=1', 'foo'],
[('text/html', 1.0)],
),

This comment has been minimized.

@whiteroses

whiteroses Sep 6, 2018
Contributor

This test is under the test class TestAcceptValidHeader — could the AcceptNoHeader and AcceptInvalidHeader tests be moved to their respective test classes? And if we create the header within the test as in the other tests, we can remove the header test parameter, save on repetition, and the AcceptNoHeader and AcceptInvalidHeader can then be tested with the exactly same four cases as well?

This comment has been minimized.

@mmerickel

mmerickel Sep 6, 2018
Author Member

I'd be fine with moving the test to somewhere else but I want the examples kept together because the point of this test is to make sure that all 3 variants filter invalid offers in the same way.

This comment has been minimized.

@whiteroses

whiteroses Sep 6, 2018
Contributor

But we could say that about all the other tests in the module where the behaviour needs to be consistent across the three header classes, so all the tests would need to be bound together. If you want that to be changed for all the tests to make sure they don't go out-of-sync with each other, that can be done, but as things are right now this test stands out as completely different from how all the tests around it are done and organised, such that we (or anyone else) wouldn't know to look for the tests for AcceptInvalidHeader and AcceptNoHeader under TestAcceptValidHeader.

Or if this is uniquely important, I could submit a PR to move the tests to their own test class?

This comment has been minimized.

@mmerickel

mmerickel Sep 6, 2018
Author Member

it made sense to me to group them, but if it's inconsistent with your test suite then I think it's fine if you want to break them apart

This comment has been minimized.

@whiteroses

whiteroses Sep 6, 2018
Contributor

Very much understand your reasoning, it's just that that's not how all the other tests are arranged. Will submit PR tomorrow - thanks.

This comment has been minimized.

@whiteroses

whiteroses Sep 7, 2018
Contributor

@bertjwregeer But is that still binding the tests for AcceptValidHeader, AcceptNoHeader and AcceptInvalidHeader together in TestAcceptValidHeader? I was just going to place the tests in TestAcceptValidHeader, TestAcceptNoHeader and TestAcceptInvalidHeader, as has been done for all the other tests in the rest of the module. I understand the reasoning behind keeping the tests together to ensure a consistent API, but then we'd need to go through all the other tests for all the four headers and do the same?

This comment has been minimized.

@bertjwregeer

bertjwregeer Sep 7, 2018
Member

That's fine for now, but we may be able to have some tests that are collapsed so that we can make sure there are no regressions moving forward where someone accidentally changes something in 2 of the 3 for example.

This comment has been minimized.

@whiteroses

whiteroses Sep 7, 2018
Contributor

I submitted the PR to move the tests into their separate test classes for now. Should we open an issue for binding the tests together? If you don't consider it urgent, I can work on it when I have time. I'm thinking move all Accept-header-related tests into one TestAccept class, and test 2 or 3 of the classes together where appropriate — is that what you have in mind?

This comment has been minimized.

@mmerickel

mmerickel Sep 8, 2018
Author Member

I think the grouping on these few tests was just to ensure consistency on the same inputs being filtered the same way. I don't have a strong opinion on reorganizing the rest of the suite.

This comment has been minimized.

@whiteroses

whiteroses Sep 10, 2018
Contributor

It seems like it'd be a good idea, as we need the api to be consistent across the three classes, and I think there are more methods like this one that would benefit from having the tests be grouped together. There are other, more urgent issues right now that I'd like to work on first, but I can submit a PR at some point so you can see if you think it helps.

@whiteroses
Copy link
Contributor

@whiteroses whiteroses commented Sep 6, 2018

Thank you for fixing this @mmerickel!

bertjwregeer added a commit that referenced this pull request Oct 15, 2018
do not raise on offers that do not match the media type ABNF
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants