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

Support checking multiple requirements #54

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

shizmob
Copy link

@shizmob shizmob commented Jan 4, 2022

This patchset changes the router to be able to handle multiple requirements.
It does this by adding a small change to the matching core to iterate over the requirements instead of doing a direct match,
and by making a change to the group merging logic: instead of the presence of absence of requirements, it will check for a potential overlap in the keys being checked. If there is no overlap, there's a potential for ambiguous handlers, so we need to reject it.

As a side-effect, this also now allows the router to match situations like this:

@app.get('/foo')
def handler1(request):
    ...

@app.get('/foo', requirements={'a': 42})
def handler2(request):
    ...

Where handler1 will be invoked if a is unset or not 42, and handler2 if it is.

@ahopkins ahopkins marked this pull request as draft January 5, 2022 09:42
@ahopkins ahopkins changed the title DRAFT: Support checking multiple requirements and their absence Support checking multiple requirements Jan 5, 2022
@ahopkins
Copy link
Member

ahopkins commented Jan 5, 2022

As a side-effect, this also now allows the router to match situations like this:

Where handler1 will be invoked if a is unset or not 42, and handler2 if it is.

How will it handle a = 41?

@shizmob
Copy link
Author

shizmob commented Jan 5, 2022

How will it handle a = 41?

This matches the or not 42 case, so it will invoke handler1.

@ahopkins
Copy link
Member

@shizmob What is the status of this?

@ahopkins
Copy link
Member

@shizmob?

@shizmob
Copy link
Author

shizmob commented Nov 23, 2022

Sorry, this issue completely dropped of my radar as I had to dedicate myself towards some other projects. Is there anything from your side that you'd like clarified or changed in this PR?

@ahopkins
Copy link
Member

Excellent, thanks. Can you merge whatever latest is needed and mark as ready for review?

@shizmob shizmob marked this pull request as ready for review November 23, 2022 15:59
@shizmob
Copy link
Author

shizmob commented Nov 23, 2022

Done! I added another change to the requirements, as pytest-flake8 exhibits compatibility issues with flake8 5: tholo/pytest-flake8#87. flake8 4.x seems to be broken on Python 3.12 though, so we may need to re-think the current approach to invoking flake8. Since we're already using black, does it make sense to drop it entirely?

@ahopkins
Copy link
Member

Going to hold off on this until after the LTS goes out.

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

Successfully merging this pull request may close these issues.

None yet

2 participants