Conversation
xni
force-pushed
the
deprecated-by
branch
2 times, most recently
from
2ae4229 to
cb823e9
Compare
di
left a comment
di
left a comment
There was a problem hiding this comment.
Added a couple of small nits, but one big thing we want to address here:
It's possible that a deprecated classifier might be replaces by multiple new classifiers, e.g. if a classifier is not specific enough and we need to split it into two more specific classifiers.
I mentioned this in the parent issue but it was easy to overlook:
It would be great if we could optionally identify one or more classifiers which replace a deprecated classifier when deprecating it
I think this is going to require some significant changes to this PR, so I'll hold off on additional review until we address that.
| pretend.call( | ||
| ( | ||
| "Deprecated classifier 'Classifier :: For tSeting' " | ||
| "in favour of 'Classifier :: For Testing'" |
There was a problem hiding this comment.
Could you use American-English spelling here for consistency? Sorry! 🙂
| # deployment process, but while the previous version of the code is still | ||
| # up and running. Thus backwards incompatible changes must be broken up | ||
| # over multiple migrations inside of multiple pull requests in order to | ||
| # phase them in over multiple deploys. |
There was a problem hiding this comment.
Can you remove the comments from this migration?
warehouse/admin/views/classifiers.py
Outdated
| # in favour of classifier A, this will create a circular dependency. | ||
| message = ( | ||
| f"You can not deprecate the classifier " | ||
| f"in favour of already deprecated classifier" |
There was a problem hiding this comment.
American-English here as well please.
| </option> | ||
| {% endfor %} | ||
| </select> | ||
| <label for="deprecated_by">Deprecated in favour of (optional):</label> |
There was a problem hiding this comment.
American-English here as well please.
| <select name="deprecated_by" id="deprecated_by"> | ||
| <option selected>(Empty)</option> | ||
| {% for classifier in classifiers %} | ||
| <!-- TODO: mark currently selected package as disabled --> |
There was a problem hiding this comment.
I'm not sure I understand this TODO. Can you either resolve or remove it?
|
@di, thanks a lot for your comments! I will try to address them asap |
|
@di , Dustin, I've updated my PR, please, take a look when you have a moment! Thank you! |
warehouse/forklift/legacy.py
Outdated
| alternatives = get_alternatives( | ||
| request.db.query(Classifier) | ||
| .filter(Classifier.classifier == first_invalid_classifier) | ||
| .limit(1) |
There was a problem hiding this comment.
The .limit filter is unnecessary if you're using .one below.
warehouse/forklift/legacy.py
Outdated
| first_invalid_classifier = min(invalid_classifiers) | ||
| host = request.registry.settings.get("warehouse.domain") | ||
| classifiers_url = request.route_url("classifiers", _host=host) | ||
| alternatives = get_alternatives( |
There was a problem hiding this comment.
I think this should be as simple as:
alternatives = (
request.db.query(Classifier)
.filter(Classifier.classifier == first_invalid_classifier)
.one()
.alternatives
)We don't need to also check if the alternatives are deprecated and have alternatives (although I appreciate you being thorough).
warehouse/forklift/legacy.py
Outdated
| f"deprecated, see {classifiers_url} for a list of valid " | ||
| "classifiers." | ||
| f"deprecated{alternative_classifiers}. " | ||
| f"See {classifiers_url} for a list of valid classifiers." |
There was a problem hiding this comment.
I think this validation message should say:
Classifier "Whatever" has been deprecated, see https://pypi.org/classifiers/ for a list of valid classifiers.
when there is no alternative, or
Classifier "Whatever" has been deprecated in favor of the following classifier(s): "Something", "Another".
when there are alternatives. The logic can just be simplified to:
if alternatives:
raise ...
else:
raise ...
warehouse/admin/views/classifiers.py
Outdated
| try: | ||
| if deprecated_classifier_id in alternative_classifier_ids: | ||
| raise ValidationException( | ||
| f"You can not deprecate the classifier in favor of itself" |
There was a problem hiding this comment.
Let's just remove the classifier from the list of possible alternative classifiers, and don't worry about explicitly checking it here. Because this UI is just use by the administrators (and relatively infrequently, at that) we don't need to worry too much about these edge cases, I'd rather just simplify the logic as much as possible.
warehouse/admin/views/classifiers.py
Outdated
| f"{deprecated_classifier.classifier!r} " | ||
| f"in favor of already deprecated classifier " | ||
| f"{alternative_classifier.classifier!r}" | ||
| ) |
There was a problem hiding this comment.
Ditto here. Let's just filter deprecated classifiers in the UI.
|
|
||
| request.session.flash( | ||
| f"Deprecated classifier {classifier.classifier!r}", queue="success" | ||
| ) |
There was a problem hiding this comment.
I think we can just leave the flash message as-is.
|
@di , whenever you have a time, please, take a look. Simplified things as much as possible :) |
|
@xni I see you have a merge conflict here; could you take a look, and leave a comment when you resolve it? Thanks! |
|
Note: |
|
Hi @xni, wanted to let you know that we went a slightly different direction by making the classifiers external to PyPI entirely, which removed the requirement for this feature in Warehouse. Thanks for your work here, though! Please let us know if you're interested in working on another feature. |
4 participants
This is a part1 of #4626 feature, which allows admin to specify the alternative.
I will update this PR or place a new one with API update, that shows the alternative to the package maintainer.