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

Awkward wording in docstring for find_matches() #137

Closed
brettcannon opened this issue Sep 2, 2023 · 5 comments · Fixed by #138
Closed

Awkward wording in docstring for find_matches() #137

brettcannon opened this issue Sep 2, 2023 · 5 comments · Fixed by #138

Comments

@brettcannon
Copy link
Contributor

The docstring says the identifier parameter "identifies the dependency matches of which should be returned." Is this trying to say it "it identifies the dependency for which matches should be returned"?

@uranusjr
Copy link
Member

uranusjr commented Sep 6, 2023

It’s trying to say (iirc) for each candidate returned by find_matches, identify(candidate) should return the same value as identifier. A rewording would be helpful…

@brettcannon
Copy link
Contributor Author

Does that mean identify() can be called on a candidate as well? The identify() docstring only mentions requirements:

"""Given a requirement, return an identifier for it.
This is used to identify a requirement, e.g. whether two requirements
should have their specifier parts merged.
"""

The typing suggests it can be either, but the README says " "requirement" ... SHOULD NOT be used when describing a Candidate, to avoid confusion", so that docstring might also be misleading.

And I'm happy to submit a PR to clear up the wording on all of this once I understand exactly what's expected.

@uranusjr
Copy link
Member

uranusjr commented Sep 7, 2023

Yes; the argument is called requirement_or_candidate (and typed as Union[RT, CT]). The docstring does indeed need some fixup.

@brettcannon
Copy link
Contributor Author

I'll create a PR to try to clarify both docstrings.

@brettcannon
Copy link
Contributor Author

#138 is my attempt to clarify things.

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 a pull request may close this issue.

2 participants