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

Add method_or_response= as optional keyword parameter for add() method. #737

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

Conversation

paulsuh
Copy link

@paulsuh paulsuh commented Sep 9, 2024

Fixes issue #734

Adds a keyword parameter method_or_response to the add(...) method. The new keyword parameter is added as the last keyword parameter so that existing code that treats the first and second positional arguments as being assigned to the parameters method and url respectively continues to work without modification.

Unit test of the new functionality (all tests passing) and documentation in the README.rst have been added.

@paulsuh paulsuh changed the title Add method_or_response= as possible keyword parameter for add() method. Add method_or_response= as optional keyword parameter for add() method. Sep 9, 2024
@@ -778,6 +778,7 @@ def add(
url: "Optional[_URLPatternType]" = None,
body: "_Body" = "",
adding_headers: "_HeaderSet" = None,
method_or_response: "_HTTPMethodOrResponse" = None,
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't adding a method parameter to upsert() get the parameter name alignment you're looking for? We would also need to possibly update replace() and remove(). I think adding a shorter and easier parameter name to those methods is better than expanding add(). The .add() method is by far the most frequently used method of the set and its conventions will be the ones that folks remember.

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