Skip to content

Add PackageFinder.make_candidate_evaluator() #6687

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 2 commits into from
Jul 11, 2019

Conversation

cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Jul 7, 2019

This is a follow-on to the refactoring in PR #6684.

The main thing this PR does is add a make_candidate_evaluator() method to the PackageFinder class to parallel the make_link_evaluator() method. This sets things up to allow for per-requirement CandidateEvaluator objects, which will be useful for later PR's.

This PR also does the following related things:

  1. Add a CandidatePreferences class to encapsulate most of the options needed for CandidateEvaluator. (A later PR can add a parallel LinkPreferences class for LinkEvaluator.) Both of these will help to cut down the number of arguments / attributes needed for the PackageFinder class. It also helps the reader to conceptualize what "package-finding" stage the various options / attributes are needed for.
  2. Refactors out from CandidateEvaluator.make_found_candidates() a separately testable CandidateEvaluator.get_applicable_candidates() method. This isolates to one method the logic for filtering the "non-applicable" candidates out from the list of all candidates. This PR also adds a unit test of this method.

@cjerdonek cjerdonek added C: finder PackageFinder and index related code skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code labels Jul 7, 2019
Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

Have we considered going the direction of the caller passing in a CandidateEvaluator factory instead of bundled preferences/options used by PackageFinder itself to instantiate CandidateEvaluator(s)? We would still get to the eventual goal of per-requirement CandidateEvaluator objects, and at the same time reduce some of the boilerplate related to translating arguments between different structures.

@classmethod
def create(
cls,
target_python=None, # type: Optional[TargetPython]
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, what is the style convention for the offset of these type declaration comments? In some places it is aligned with the others in a declaration, in others there are chunks of aligned comments, and in others they are all just 2 spaces from their corresponding parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know of one. I've always been accustomed to two spaces for inline comments, but some people started aligning them. I think it's overboard to always align (e.g. having to change every argument line just because of a change in one line), so I sometimes align to adjacent lines when convenient and it improves the readability / how it looks.

@cjerdonek cjerdonek force-pushed the add-candidate-preferences branch from 071b99d to 4a9e306 Compare July 7, 2019 21:36
@cjerdonek
Copy link
Member Author

Have we considered going the direction of the caller passing in a CandidateEvaluator factory instead of bundled preferences/options used by PackageFinder itself to instantiate CandidateEvaluator(s)?

For now I just wanted to keep things unclever while also keeping things flexible and not making changes that would require unneeded changes to test code. I did briefly consider CandidateEvaluatorFactory but thought that introduced too much abstraction for not so much benefit. Things are still changing, so we can always revisit if we find that something will provide advantages.

Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

LGTM.

@cjerdonek
Copy link
Member Author

Thanks for the review and helpful comments, @chrahunt!

@cjerdonek
Copy link
Member Author

(Merging this now so I can work on the follow-on PR.)

@cjerdonek cjerdonek merged commit a9bf041 into pypa:master Jul 11, 2019
@cjerdonek cjerdonek deleted the add-candidate-preferences branch July 11, 2019 02:58
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Aug 10, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 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 C: finder PackageFinder and index related code 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