-
-
Notifications
You must be signed in to change notification settings - Fork 345
WIP: Implement a "Replaced by" relationship #1888
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
Conversation
f6bb8d1
to
7744fd4
Compare
There were a whole bunch of files with duplicated listings in the csproj file for tests. I removed all the ones that didn't exist. If anyone understands how to make these tests work, please help!!!! I can't get my head around them. |
03a85ed
to
ed0c59c
Compare
Spec.md
Outdated
selected for updating, while this mod is uninstalled. If this mod identifier | ||
is brought back to life, an epoch change should be applied. A *replaced_by* | ||
relationship should be added to the final release of the mod being replaced. | ||
The listed mod should include a "provides" relationship either to this mod, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stuff about "provides" makes this sound complicated, since now I have to worry about two changes to two different mods. Will replaced_by
work on its own? What happens if the provides relation is missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, does this advice apply to the multiple-replacement case? Presumably if a mod only replaces part of another mod, it would not be correct to say that it "provides" that mod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no multiple-replacement case. We are only permitting a singe replacement mod.
Replaced_by works on its own. This is a "should", not a "must".
} | ||
|
||
|
||
public int RunCommand(CKAN.KSP ksp, object raw_options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale for making a new command for this? From reading #904, it sounded like this would tie into the existing upgrade
command as needed. If we're confident enough in the replacement mod to put it in the metadata, why would we want to trouble the user with knowing whether it's an upgrade or a replaced_by?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is sufficiently different that users should have to make a distinct choice to replace a mod rather than simply upgrading. We don't want users asking us why they have unknown mods in their list suddenly when they aren't dependencies. Also, the logic is quite distinct and it's way easier to implement as a separate command than increase the complexity of the existing command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points. But if a user just wants to get the latest updates and doesn't particularly care if they're called XYZ or XYZReplaced, I could see it being an irritation to have to run multiple commands, every time.
What about using an optional flag to create a combination command? Something like:
ckan upgrade --all --with-replacements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions to help me understand the project.
3ea1902
to
955a0bd
Compare
@HebaruSan , I think I've addressed all the issues you've raised so far now. Can you add a consoleui implementation for me? |
Sure; politas#6 now has the changes I had for that. I'm not sure that it's 100% robust, but it should at least cover the basics. |
@HebaruSan , I'm finding it very hard to find time to get back in and finish this. If you would like to take it over and bring it to completion, we might actually be able to get this feature included. |
Closing in favour of #2671 |
I welcome suggestions and comments as I work on this.
Tracking steps top level
Closes Create a replaced_by relationship to allow seamless continuation of deprecated mods #904