Skip to content

Call into operations.generate_metadata to generate metadata #7051

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

Merged
merged 3 commits into from
Sep 21, 2019

Conversation

pradyunsg
Copy link
Member

I'll start moving all the code into this module in follow up PRs.

As things stand, it'll completely delegate all the metadata generation
to InstallRequirement's methods.

Follow ups will move related code into this module.
@pradyunsg pradyunsg added type: refactor Refactoring code skip news Does not need a NEWS file entry (eg: trivial changes) labels Sep 20, 2019
@chrahunt
Copy link
Member

IMO doing cleanup in-place or extracting functionality as free functions that take a the minimum required arguments would be easier to follow and require less effort to review. I'm going back and forth on this because it's hard to see where it can go.

@pradyunsg
Copy link
Member Author

My plan is to be moving the methods to become functions, in the new module, in follow up PRs.

There's a decent amount of code that's there only for these calls, enough that I want to split the adapt-existing-code, move-logic and cleanup-logic steps.

@pradyunsg
Copy link
Member Author

@chrahunt are you suggesting to add moving the methods that are currently being directly called, to this PR?

I'm not sure how to address that -- I guess I'm trying to do too less in this PR?

@chrahunt
Copy link
Member

I guess I'm trying to do too less in this PR?

That could be it. With this change on it's own I'm concerned about the proliferation of InstallRequirement into more modules. I could comment on that, but then we might spend more time than it's worth when this is a very small part of your overall refactoring and the motivation would be more obvious if there were more changes.

The alternative I suggested above was a way to avoid using InstallRequirement in more places, I should have been explicit about that to begin with.

@pradyunsg
Copy link
Member Author

Ah. Yes.

My plan is to move the code in these methods (basically cut-paste-clean the methods being called) and then have Distribution objects call these methods, resulting in the removal of metadata generation logic from InstallRequirement.

The methods being called would basically move over/be removed in follow ups.

@pradyunsg
Copy link
Member Author

Cool, so I'm understanding that your concern is to avoid using InstallRequirement in more places -- that makes sense and I don't want to either.

I'll address that pretty soon in follow up PRs.

@pradyunsg
Copy link
Member Author

I'm merging this since I don't think you're opposed to the broader changes this is a part of.

@pradyunsg pradyunsg merged commit 7fabb16 into pypa:master Sep 21, 2019
@pradyunsg pradyunsg deleted the refactor/metadata-generator branch September 30, 2019 10:46
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Oct 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants