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

Bad type in get_or_create docs #371

Closed
eranzim opened this issue Oct 7, 2018 · 5 comments
Closed

Bad type in get_or_create docs #371

eranzim opened this issue Oct 7, 2018 · 5 comments
Labels
documentation Issues requiring modifications to the documentation

Comments

@eranzim
Copy link

eranzim commented Oct 7, 2018

I believe there is a mistake in the docs of get_or_create class method in core.py. They say:

        :param props: dict of properties to get or create the entities with.
        :type props: tuple

Passing a tuple results in an error (AttributeError: 'tuple' object has no attribute 'get')... I believe it should be dict, like the description says.
Being as it is, PyCharm falsely alerts me I'm passing a wrong type in all my get_or_create calls.
Thanks!

@leonaidus
Copy link
Contributor

leonaidus commented Nov 20, 2018

@eranzim This representation is correct as it takes tuple of dictionaries.
To understand this fact try below code -

def myfunc(*props):
    return type(props)

print(myfunc({'name': 'Leonaidus'}))

This gives us

<class 'tuple'>

which is python behaviour for Non-Keyword Arguments

@eranzim
Copy link
Author

eranzim commented Nov 25, 2018

I understand, and this makes sense that args would be a tuple inside the function. Does that mean that there is a bug in PyCharm? Because even in the example you gave, it will issue a warning if I add :type props: tuple in the docs, but not if I add :type props: dict.. It makes sense to me that since args will always be a tuple inside the function, the type specification in the docstring probably refers to the elements inside the tuple. My opinion is reinforced when I pass multiple dicts to myfunc. With :type props: dict there is no warning, but with :type props: tuple PyCharm marks each dict separately (rather than just the whole argument list) and issues a type mismatch warning.

@aanastasiou aanastasiou added the documentation Issues requiring modifications to the documentation label Mar 5, 2019
@aanastasiou
Copy link
Collaborator

@eranzim Thanks, it is worth the clarification. Yes, the type right there would be tuple and equivalent to a call to a dict like params.items().

@aanastasiou
Copy link
Collaborator

@eranzim This has now been changed to:

        :param props: Arguments to get_or_create as tuple of dict with property names and values to get or create 
                      the entities with.
        :type props: tuple

Not sure about what PyCharm uses for code analysis and whether this should be looked any further though.

@dixanms
Copy link

dixanms commented Mar 27, 2020

As Python always packages function arguments as a tuple when function uses *some_name to declare an undetermined number arguments, the convention seems to be to use any documented type (docstring :type) as the type of tuple's contained elements.

PEP 484 suggests that

I reached out to JetBrains regarding this and this is their response:

"one is not really expected to annotate arbitrary (*args) or default (**kwargs) arguments as tuples or dicts. As you said they are always tuples or dicts so the static analysis tools (or engineers reading the code) always know the collection type.

The part of interest is the tuple/dict items type. See a relevant part of PEP 484 https://www.python.org/dev/peps/pep-0484/#arbitrary-argument-lists-and-default-argument-values I guess we can extrapolate the same logic to type annotations in docstrings."

Accordingly, what makes sense is to change get_or_create and create_or_update methods to

  :type props: dict

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues requiring modifications to the documentation
Projects
None yet
Development

No branches or pull requests

4 participants