Fix deprecations #1269
Fix deprecations #1269
Conversation
|
In addition to reviewing the code the actual idea behind the changes should be verified. Do we want to mark any mods with deprecated dependencies as deprecated? The old implementation implied that's what's supposed to happen, but then didn't quite work that way. I.e. after the changes people will see more mods deprecated than before (or likely won't see them, since they will be properly filtered out in the OnlineModList. |
anttimaki
force-pushed
the
indexed-package-db
branch
from
b745c05
to
788cd1d
Compare
The old implementation had multiple bugs, and as a result the deprecation status of dependency chains we're not properly checked. Fixing this will cause many mod packs to start showing as deprecated.
After earlier refactorings, the map was used only by the internal methods anymore, so there was no performance benefits to be had by having it stored in memory. Passing the map as an argument makes the code easier to follow, test, and refactor.
Use precalculated deprecation map that takes into account dependency deprecations on LocalModCard. Use the same map on OnlineModView to filter out deprecated mods unless user uses filters to specifically ask for them. And finally use the same map, via Vuex getter, to show the number of mods available online on the navigation sidebar. This works a bit oddly since the filter for deprecated mods is taken into account, but other filters (e.g. NSFW content) are not. However, this is how it worked before the changes too, and further changes are considered outside the current scope.
I suspect hiding them from listings when a dependency is deprecated but the main package isn't is overkill at least for now (as it'd be potentially disruptive & breaking backwards-compat), but we should ideally display that somehow on the listing (e.g. a yellow warning sign for issues with dependencies with a mouse-over tooltip that notes some of the dependencies are deprecated) How big of an effort would implementing something like that be? |
There was a problem hiding this comment.
Highly relevant to this change, there are some other considerations the current logic fails to account for.
Lets start with noting that at least the following different "problems" can exist for any given package that's returned by the package list API:
- The package is deprecated
- The package has a dependency that's deprecated
- The package has a dependency that's unavailable (rejected, removed, or in another community)
The logic that deals with deprecations & indicating their status on the UI should ideally account for all of these problem scenarios in some fashion. Additionally, we might want to have other similar status attributes for packages in the future (for example, "verified" packages) which might also benefit from bubbling upwards (in the case of verified mods, perhaps it would be worth making a distinction between modpacks that only use verified dependencies and ones that don't)
All of the above taken into account, it's pretty clear that the way the system currently works is less than ideal. As for how much we should address in this specific PR already I'm not entirely sure.
I suspect it could be best to keep the behavior the same as it has been before if it's easy enough to do & do an improvement pass later on that adds support for multiple different statuses instead of a boolean deprecated status.
I think my thoughts condensed amount to: Marking mods deprecated that shouldn't be deprecated is worse than not marking mods deprecated that should due to dependencies.
Also just to illustrate an interesting potential issue, consider a package that gets flagged as deprecated due to a dependency marked deprecated, and then later on suddenly becomes undeprecated because the deprecated dependency is removed entirely. Right now there would be no warning sign to the users that the package is not in fact fine.
IDK but I'd suspect rather big since then we'd have to keep the old broken implementation (and refactor it to fit the other changes in this PR stack) and the new implementation and use one in some places and the new in others.
As above, IDK how easy doing that would be since other changes have been built on top of these in the past month.
I'm not sure what this refers to. I agree with the statement, but as far as I can remember, the changes in this PR doesn't cause false positives, but removes false negatives. So more relevant statement would be "Not marking mods deprecated that should be due to dependencies is worse than the code to actually do what it was intended to do". EDIT: to clarify, both adding the more granular state for mods (which I agree would be a good change), and not fixing the bug to keep things as they were, can be done if you deem them worth the non-trivial effort. The former would a PR separate from this stack, but would still require the same changes to stack as the latter one does (this is a gut feeling). EDIT2: I'm not sure if this would work, but I guess I could "break" the new implementation so it doesn't actually check the dependencies for deprecations. I think that should be similar behaviour to the old implementation. If not 100% same, at least close. Doing this and keeping the changes otherwise intact should required only small effort. |
|
#1295 changes the new method to work (AFAICT) like the old broken one did, while requiring minimal changes to PR stack. Showing other states and refixing the method should be done in a separate PR. |
No description provided.