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.

Sign up for GitHub

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

Jump to bottom

Remove unused comes_from argument of parse_requirements #8551

Merged
merged 1 commit into from
Nov 8, 2020
Merged

Remove unused comes_from argument of parse_requirements #8551

merged 1 commit into from
Nov 8, 2020

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Jul 6, 2020 •
edited
Loading

No caller of parse_requirements ever passes the comes_from argument.
Its use is unclear and if it was meant as a base url, it is probably broken.
I therefore propose to remove it to clarify the code and avoid confusion with other uses of the comes_from term elsewhere.

@sbidoul sbidoul added skip news Does not need a NEWS file entry (eg: trivial changes) and removed skip news Does not need a NEWS file entry (eg: trivial changes) labels Jul 6, 2020
@sbidoul sbidoul marked this pull request as ready for review July 6, 2020 23:07
uranusjr
uranusjr approved these changes Jul 7, 2020
View reviewed changes
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

If the naming is any indication, comes_from should be Optional[InstallRequirement] that indicates “why” a dependency is needed. This should always be None for ParsedRequirement since they always come from requirements.txt files, i.e. are always user-supplied. Removing the argument makes sense to me.

@pradyunsg
Copy link
Member

I think it was supposed to be the name of the specific requirements file that it is sourced from?

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 8, 2020
@sbidoul
Copy link
Member Author

sbidoul commented Nov 8, 2020

Rebased. It should be good to go.

@uranusjr uranusjr removed the needs rebase or merge PR has conflicts with current master label Nov 8, 2020
@pradyunsg pradyunsg merged commit a4f4bfb into pypa:master Nov 8, 2020
@sbidoul sbidoul deleted the rm-comes-from branch November 8, 2020 22:31
@AndydeCleyre AndydeCleyre mentioned this pull request Nov 11, 2020
Merged
3 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@uranusjr uranusjr uranusjr approved these changes

Assignees
No one assigned
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sbidoul @pradyunsg @BrownTruck @uranusjr