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

Remove context keyword from security handlers #1690

Merged

Conversation

Ruwann
Copy link
Member

@Ruwann Ruwann commented Apr 21, 2023

Follow-up on #1671

Since the request context is available globally, we can remove this complexity from the security handlers ("There should be one-- and preferably only one --obvious way to do it.")

In addition it seems like there weren't any tests for the context in security handler functions, and I don't think there's a lot of value in a test for checking the context in security functions specifically.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4767722029

  • 0 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 93.945%

Totals Coverage Status
Change from base Build 4763817389: 0.04%
Covered Lines: 3305
Relevant Lines: 3518

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4767722029

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 93.945%

Totals Coverage Status
Change from base Build 4763817389: 0.04%
Covered Lines: 3305
Relevant Lines: 3518

💛 - Coveralls

Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thx @Ruwann!
So should we do this for the view functions as well?

@RobbeSneyders RobbeSneyders merged commit 97e8a8e into spec-first:main Apr 22, 2023
5 checks passed
@Ruwann
Copy link
Member Author

Ruwann commented Apr 22, 2023

I think it would indeed make sense to also do it for the view functions as well. I haven't found the reasoning for adding it but the same argument applies: one way of accessing the context instead of two.

For the security handlers, the context argument was added in #869 but not really clear what the reason was - I assume it was because it was the easiest (?) way of making the context available for aiohttp in security handler functions?

@Ruwann Ruwann deleted the feature/remove-context-in-security branch April 23, 2023 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants