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

Restrict methods #543

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Restrict methods #543

wants to merge 4 commits into from

Conversation

Olegt0rr
Copy link
Collaborator

@Olegt0rr Olegt0rr commented Dec 2, 2024

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

Copy link

codecov bot commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (b6883cb) to head (d88dd03).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #543   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          475       487   +12     
  Branches        17        18    +1     
=========================================
+ Hits           475       487   +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Dreamsorcerer
Copy link
Member

I think that's overkill, we only want to ensure that the user understands that GET is the only allowed method for browsers. A boolean should suffice.

@Olegt0rr
Copy link
Collaborator Author

Olegt0rr commented Dec 3, 2024

Switched to bool

@Dreamsorcerer
Copy link
Member

I've added changelog as we don't seem to have any docs. I think that's fine, so feel free to merge if you're happy with it.

@Olegt0rr Olegt0rr marked this pull request as ready for review December 5, 2024 03:16
@Olegt0rr
Copy link
Collaborator Author

Olegt0rr commented Dec 5, 2024

@Dreamsorcerer, thanks

I got some thoughts :D

This way we helped to warn frontend developer with 405 status code after request.
But we still not warn backend developer. May be we need to also add a warning.warn?
Or fail not as answer to the front, but with RuntimeError?

@Dreamsorcerer
Copy link
Member

But we still not warn backend developer. May be we need to also add a warning.warn?

I would expect a backend developer to test their code, even if only with Postman or similar, which is why this probably still works.
But, if you have a suggestion to create an error for the developer earlier, then that'd be even better.

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.

2 participants