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

Parameter 'class_or_builder' unfilled #228

Open
sudosu98 opened this issue Jul 19, 2021 · 7 comments
Open

Parameter 'class_or_builder' unfilled #228

sudosu98 opened this issue Jul 19, 2021 · 7 comments
Milestone

Comments

@sudosu98
Copy link

class Test(Consumer):
@returns.json
@JSON
@patch("orders/address/pickup")
def task_updatePickupLoc(self, **body: Body):
"""Updates Pickup Location of a given order"""
When the method is called using required parameters the code runs and performs a patch request but gives me a warning to provide 'class_or_builder' why?

@bjornsnoen
Copy link

@prkumar care to weigh in? This looks like it affects anything that inherits from decorators.MethodAnnotation. I'm getting this warning from pyright, so you'll probably be able to easily reproduce using pylance in vscode as well.

image

@bjornsnoen
Copy link

bjornsnoen commented Feb 23, 2022

You can also reproduce with a toy example like this:

from uplink import Consumer, returns

@returns.json
class MyAPI(Consumer):
    """Reproduces"""

my_api = MyAPI(base_url="https://example.com/")

That one gets the same error because @returns.json is decorating the __init__ method, which is probably a bug in itself, as it means you can't use the trick of putting method decorators on the class as a shortcut as described in the docs.

@prkumar
Copy link
Owner

prkumar commented Feb 23, 2022

Hey @bjornsnoen - I'm unable to reproduce this on the latest version of uplink, What version are you using?

@bjornsnoen
Copy link

bjornsnoen commented Feb 23, 2022

@prkumar I made a small repo that reproduces it, here https://github.com/bjornsnoen/uplink-toy
I'm using version 0.9.6, which I believe is the latest version.

@prkumar
Copy link
Owner

prkumar commented Feb 24, 2022

@bjornsnoen - Thanks for providing the helpful sample repo. I was able to reproduce locally using it! A couple quick thoughts:

  1. If I understand correctly, this sounds like an issue that only happens when running a type checker (specifically pyright) against code that uses the library, right? In other words, your example should run fine without type checking, right?

  2. As far as I'm aware, we don't boast any support for pyright or mypy in the docs, right? Regardless, it makes sense why you would expect this to work. I'm just making sure that we are not advertising support, since we clearly don't support those type checkers.

  3. IIUC, to fix this issue, we would need to provide a type hint that helps pyright understand how to interpret the usage of MethodAnnotation subclasses as class decorators. More specifically, in your example, it seems like pyright is interpreting the type of MyAPI incorrectly: it believes that MyAPI is not a class, since it was wrapped by returns.json.

@bjornsnoen
Copy link

@prkumar yes, you understand correctly, the code works as expected and the only reason to expect static typing to work is because things like the returns.from_json decorator actively use type annotations. If you search the docs for "typing" some things do come up, the top hit seeming to be a core python module for PEP 484?

I want to make clear that the @returns.json decorator isn't only broken when used to decorate classes, it does the same when decorating class methods, like this:

from uplink import Consumer, get, returns


class MyAPI(Consumer):
    """Reproduces"""

    @returns.json
    @get("test")
    def test():
        """Also reproduces"""


my_api = MyAPI(base_url="https://example.com/")
my_api.test()

The call to my_api.test() gives the same error in pyright, as it now expects the test method to have a class_or_builder argument passed to it. This is much more annoying to deal with, as we don't get any help from autocomplete/intellisense with knowing which parameters an endpoint wants. IMO it would be perfectly OK to only fix this basic use case, and say flat out that if you use the decorators on a class level it will break static type checkers, if that ends up being an option.

@prkumar
Copy link
Owner

prkumar commented Feb 24, 2022

@bjornsnoen - Gotchu. As you mentioned, I think it makes sense to assume that the package supports type checking, considering the usage of type annotations throughout the docs. IIUC, to fix this, we need to add type annotations to the library. Notably, we've avoided adding these because of our advertised support for Python 2.7. However, version 0.9.* will be the last minor version to support Python 2.7. Starting with version 0.10, we can start adding type annotations in the library, which should address this issue.

@prkumar prkumar added this to the v0.10.0 milestone Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants