Skip to content
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

WIP: Implement a "Replaced by" relationship #1888

Closed
wants to merge 30 commits into from

Conversation

politas
Copy link
Member

@politas politas commented Sep 6, 2016

I welcome suggestions and comments as I work on this.

Tracking steps top level

  • Spec
  • Schema
  • Test cases
    • Test valid replacement
    • Test invalid replacement
  • Core changes
    • replaced_by added to CkanModule.cs
    • Replace method added to ModuleInstaller
    • Replace Method accepts resolver options and resolves dependencies
    • New "ModuleReplacement" structure
    • HasReplacement registry query - checks availability and KSP compatibility
  • CLI changes
    • Show Replaceable mods in list command
    • New "Replace" Command option
    • Defined options for Replace command
    • Add Replace command to main case
    • Command line resolver options passed to ModuleInstaller.Replace
  • ConsoleUI changes
    • Show replaceable mods in modlist
    • Add "Replace" tag option
    • Call ModuleInstaller.Replace for replaced mods when applying changes
  • GUI changes
    • Replace column in modlist and visual indicator
    • replaced_by data shown in Metadata panel
    • Call to ModuleInstaller.Replace for replaced mods
    • Add a "Replaceable" GUIModFilter
    • discuss Only allow a single changetype in a given changeset (I can't find an issue for this bug, but we've had it for a long time. Selecting updates and installs together causes a crash)
    • Add Right-click menu option for Replace
  • Netkan changes
    • validate replaced_by relationship identifier
  • Make sure no TODO1888 comments remain
    Closes Create a replaced_by relationship to allow seamless continuation of deprecated mods #904

@politas politas added Enhancement New features or functionality GUI Issues affecting the interactive GUI Cmdline Issues affecting the command line Core (ckan.dll) Issues affecting the core part of CKAN In progress We're still working on this Netkan Issues affecting the netkan data labels Sep 6, 2016
@politas politas added this to the 1.22.0 milestone Sep 6, 2016
@politas politas self-assigned this Sep 6, 2016
@politas politas added pull request In progress We're still working on this and removed In progress We're still working on this pull request labels Sep 6, 2016
@politas politas changed the title Implement a "Replaced by" relationship WIP-Implement a "Replaced by" relationship Sep 6, 2016
@ayan4m1 ayan4m1 changed the title WIP-Implement a "Replaced by" relationship WIP: Implement a "Replaced by" relationship Oct 2, 2016
@politas politas removed this from the 1.22.0 milestone Dec 27, 2016
@politas politas force-pushed the feature/904-replaced-by branch 2 times, most recently from f6bb8d1 to 7744fd4 Compare November 3, 2017 10:37
@politas politas added this to the 1.24.0 milestone Nov 3, 2017
@politas
Copy link
Member Author

politas commented Nov 4, 2017

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.

Cmdline/Action/List.cs Outdated Show resolved Hide resolved
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,
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member Author

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)
Copy link
Member

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?

Copy link
Member Author

@politas politas Nov 4, 2017

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.

Copy link
Member

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

CKAN.schema Show resolved Hide resolved
Copy link
Member

@HebaruSan HebaruSan left a 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.

@politas
Copy link
Member Author

politas commented Dec 23, 2017

@HebaruSan , I think I've addressed all the issues you've raised so far now. Can you add a consoleui implementation for me?

@HebaruSan
Copy link
Member

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.

@politas politas modified the milestones: 1.24.0, 1.26 Feb 23, 2018
@politas
Copy link
Member Author

politas commented Jan 28, 2019

@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.

@HebaruSan HebaruSan self-assigned this Jan 29, 2019
@HebaruSan
Copy link
Member

@politas, OK, let me know how #2671 looks.

@politas
Copy link
Member Author

politas commented Feb 10, 2019

Closing in favour of #2671

@politas politas closed this Feb 10, 2019
@politas politas deleted the feature/904-replaced-by branch February 10, 2019 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cmdline Issues affecting the command line Core (ckan.dll) Issues affecting the core part of CKAN Enhancement New features or functionality GUI Issues affecting the interactive GUI In progress We're still working on this Netkan Issues affecting the netkan data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a replaced_by relationship to allow seamless continuation of deprecated mods
2 participants