Skip markers #194
Skip markers #194
Conversation
frenzymadness
requested review from
fridex,
goern and
harshad16
as code owners
sesheta
added
the
size/XL
micropipenv.py
Outdated
| if isinstance(dependency_info, dict) and "markers" in dependency_info: | ||
| additional_markers[normalize_package_name(dependency_name)].append(dependency_info["markers"]) | ||
| else: | ||
| additional_markers[normalize_package_name(dependency_name)].append("skip-all-markers") |
There was a problem hiding this comment.
Thanks for the fix. The approach looks correct to me. I'm wondering if skip-all-markers could be replaced with some language-defined marker that could signalize no markers and would not be a valid marker - e.g. None (not to introduce a special value here). I see the None is handled specifically down below.
There was a problem hiding this comment.
I see. My first intention was to use True for that but neither True nor None indicates to me that this value should cause something special. For me, None is like a missing marker that should itself be skipped but without any impact on other markers. And it's used that way below when a package itself has no markers except the additional ones. We can use something like "*" to indicate that the package should always be installed but it's even worse than the self-describing value in the current implementation. I'm out of ideas. Do you have some?
There was a problem hiding this comment.
Maybe use a named object, like:
skip_all_markers = object()
[...]
additional_markers[normalize_package_name(dependency_name)].append(skip_all_markers)
[...]
if skip_all_markers in markers:There was a problem hiding this comment.
A special sentinel value like this one might make sense here, thank you. Implemented.
frenzymadness
force-pushed
the
skip_markers
branch
from
e93ac06
to
97f3894
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fridex The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
sesheta
added
the
approved
Related Issues and Dependencies
#192
This introduces a breaking change
If a dependency is required at least once without any additional markers, all the other markers are skipped and the dependency is installed unconditionally. There is also a test for that.