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 ability to use kwargs for use with decorator #11

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

Conversation

sukram230799
Copy link
Contributor

Hello @bkryza

I have noticed that I'm unable to use keyword args for the decorators. I hope the examples provide an insight in what I want to achieve.

@GET('/users')
@query('search')
@query('limit')
def search_user(search, limit=10):
    pass

Ordered Parameters: works

client.search_user('123', 20)

I think this is one/the "decorest" way: works

client.search_user('123', query={'limit': 20})

But I would like to use keyword arguments: doesn't work

client.search_user('123', limit=20)
# or
client.search_user(search='123', limit=20)

I have submitted a possible fix for it. But I would consider it more of a workaround, as it manipulates a pass by reference array (kwargs). But I have submitted it anyway as a point to start a discussion.

Additionally I would like to add some test cases. Where should I put them?

Thanks
Markus

  • Use both args_dict and kwargs for _merge_args(...)
  • Remove 'used' kwargs so that they don't collide with httpx/requests

- Use both `args_dict` and `kwargs` for `_merge_args(...)`
- Remove 'used' kwargs so that they don't collide with httpx/requests
@codecov-commenter
Copy link

Codecov Report

Merging #11 (7fa2def) into master (c85fab4) will decrease coverage by 0.18%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master      #11      +/-   ##
==========================================
- Coverage   94.08%   93.89%   -0.19%     
==========================================
  Files          16       16              
  Lines         930      934       +4     
==========================================
+ Hits          875      877       +2     
- Misses         55       57       +2     
Impacted Files Coverage Δ
decorest/request.py 92.52% <80.00%> (-0.81%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@bkryza
Copy link
Owner

bkryza commented Jul 20, 2022

@sukram230799 Thanks for the PR - again :-)

The code looks ok, could you please add a test case for this for instance by duplicating these test cases with kwargs parameters in the calls, you can call it for instance test_user_methods_with_kwargs_params...

One without typing enabled:

and also this one to make sure it also works for clients with typing:

These test cases hopefully should cover args coming from both method as well as query decorators...

- Without typing
- With typing
@sukram230799
Copy link
Contributor Author

Okay I have not considered the in path case. So that fails...

    @GET('user/{username}')
    def get_user(self, username):
        """Get user by user name."""

With the testcase

res = client.get_user(username='swagger')

I'll have a look in the code on how to fix that

@sukram230799
Copy link
Contributor Author

I have found an even bigger flaw. What if we use a kwarg multiple times. I don't know what the use-case would be - but it would be valid code. As I currently delete the used kwargs we would run into a problem.

Back to the drawing board

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.

3 participants