Move mindeps generation logic to separate function #5
Conversation
Shivansh-007
commented
Jan 29, 2022
Shivansh-007
commented
|
Can you let me know if there are any updates on this PR, @FFY00? I have been waiting for a while for this functionality to be available in |
There was a problem hiding this comment.
@Shivansh-007 thank you for the contribution, and sorry for the delay 😓
FYI, in the future feel free to ping me as often as you think necessary, as that's helpful for me to not forget there's something blocked on me.
The PR looks good overall, but I have a few comments.
| def get_min_deps( | ||
| requirements: List[str], | ||
| reporter: Optional[resolvelib.BaseReporter] = None, | ||
| extras: Optional[Set[str]] = None, | ||
| markers: Optional[Dict[str, str]] = None | ||
| ) -> Dict[str, str]: |
There was a problem hiding this comment.
Let's try to avoid using specific data types unless necessary
| def get_min_deps( | |
| requirements: List[str], | |
| reporter: Optional[resolvelib.BaseReporter] = None, | |
| extras: Optional[Set[str]] = None, | |
| markers: Optional[Dict[str, str]] = None | |
| ) -> Dict[str, str]: | |
| def get_min_deps( | |
| requirements: Collection[str], | |
| reporter: Optional[resolvelib.BaseReporter] = None, | |
| extras: Optional[Collection[str]] = None, | |
| markers: Optional[Mapping[str, str]] = None | |
| ) -> Mapping[str, str]: |
| extras: Optional[Set[str]] = None, | ||
| markers: Optional[Dict[str, str]] = None | ||
| ) -> Dict[str, str]: | ||
| reporter = reporter or resolvelib.BaseReporter |
There was a problem hiding this comment.
nitpick: I'd prefer to be more explicit here, even if it takes an extra line.
| reporter = reporter or resolvelib.BaseReporter | |
| if not reporter: | |
| reporter = resolvelib.BaseReporter |
| class MinimumDependencyProvider(resolver.Provider): | ||
| def sort_candidates( | ||
| self, | ||
| candidates: Iterable[resolver.Candidate], | ||
| ) -> Sequence[resolver.Candidate]: | ||
| return sorted(candidates, key=operator.attrgetter('version'), reverse=False) |
There was a problem hiding this comment.
Is there any particular reason you moved this here? Unless there's a good reason not to, I think we should keep it in resolver.mindeps. Also notice that removing it from there breaks imports, this class is meant to be available for people to use.
| ) | ||
|
|
||
| extras = set(vars(args).get('extras', {})) | {''} | ||
| if any(arg in _MARKER_KEYS for arg in vars(args)): | ||
| extras = extras.copy() | {''} if extras else {''} |
There was a problem hiding this comment.
nitpick:
The .copy() call shouldn't be needed here because the | operator does not mutate the set, it creates a new one.
Since extras will always be set now, you could just do extras = extras | {''}. Note that we can't do extras |= {''} because |= does mutate the original set 😅
But anyway, since we are changing the type hint to Collection, we need to rewrite this snippet anyway.
| extras = set(vars(args).get('extras', {})) | {''} | ||
| if any(arg in _MARKER_KEYS for arg in vars(args)): | ||
| extras = extras.copy() | {''} if extras else {''} | ||
| if markers is not None and any(marker in _MARKER_KEYS for marker in markers): |
There was a problem hiding this comment.
nitpick: I'd skip the is None, to simplify the condition.
| if markers is not None and any(marker in _MARKER_KEYS for marker in markers): | |
| if markers and any(marker in _MARKER_KEYS for marker in markers): |
