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

Fix AD mod upgrading, add tests, and fix all warnings #3315

Merged
merged 1 commit into from
Mar 7, 2021

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Mar 7, 2021

Problem

In #3190, we allowed AD mods to be upgraded, replacing them with a CKAN-managed module.

In v1.30.0, AD mods again are no longer treated as upgradeable.

Causes

InstalledModule returns null for AD mods, and after #3307 we attempt to access properties of that return value, which throws a null reference exception. HasUpdate catches this exception and returns false.

new CkanModule[] { querier.InstalledModule(identifier).Module },

Changes

Now we pass null for the modules to remove if InstalledModule returns null. In future we would like to tell RelationshipResolver to check what will happen if we remove the DLL, but for now this will at least bring the upgrade checkboxes back.

Two tests are added to make it harder to break this accidentally:

  • Make sure that if another installed mod depends on a specific version of another mod, we do not attempt to upgrade that other mod (the original fix from Fixes for GUI and .ckan-installed modules #3307)
  • Make sure that an AD mod is treated as upgradeable when there's a compatible module with the same identifier (this fix)

Side fixes

The Tests/GUI folder is reorganized slightly, with two files moved to a new Model subfolder to match where their tested code lives.

Fixed warnings during build:

  • ModListScreen.cs(20,78): warning CS1573: Parameter '_theme' has no matching param tag in the XML comment for 'ModListScreen.ModListScreen(GameInstanceManager, bool, ConsoleTheme)' (but other parameters do)`

    Added documentation of this parameter

  • SingleAssemblyComponentResourceManager.cs(22,43): warning CS0618: 'ResourceManager.ResourceSets' is obsolete: 'call InternalGetResourceSet instead'
    SingleAssemblyComponentResourceManager.cs(46,36): warning CS0618: 'ResourceManager.ResourceSets' is obsolete: 'call InternalGetResourceSet instead'
    SingleAssemblyResourceManager.cs(20,43): warning CS0618: 'ResourceManager.ResourceSets' is obsolete: 'call InternalGetResourceSet instead'
    SingleAssemblyResourceManager.cs(44,36): warning CS0618: 'ResourceManager.ResourceSets' is obsolete: 'call InternalGetResourceSet instead'

    We were using this protected Hashtable as a cache for our ResourceSets because that's what I found on StackExchange, but the base class no longer uses this object, hence this warning. Now we use a private Dictionary instead for the same purpose.

Fixed warnings from tests:

  • 6443 [NonParallelWorker] WARN CKAN.NetKAN.Transformers.GithubTransformer (null) - Multiple assets found for 1.2 without version_from_asset

    Reworked GithubTransformerTests slightly so the multi-asset release is only processed by code that's prepared for it

  • 6609 [NonParallelWorker] WARN CKAN.NetKAN.Validators.TagsValidator (null) - Tags not found, see https://github.com/KSP-CKAN/CKAN/wiki/Suggested-Tags
    6641 [NonParallelWorker] WARN CKAN.NetKAN.Validators.TagsValidator (null) - Tags not found, see https://github.com/KSP-CKAN/CKAN/wiki/Suggested-Tags

    Added tags to the test data in CkanValidatorTests

@HebaruSan HebaruSan added Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN Pull request Tests Issues affecting the internal tests Registry Issues affecting the registry Relationships Issues affecting depends, recommends, etc. labels Mar 7, 2021
@HebaruSan HebaruSan requested a review from DasSkelett March 7, 2021 00:18
Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking very good, thanks!
(Wait with the merge until #3319 finished deployment 😁)

@HebaruSan HebaruSan merged commit 200d77b into KSP-CKAN:master Mar 7, 2021
@HebaruSan HebaruSan deleted the fix/ad-upgrade branch March 7, 2021 23:55
@HebaruSan HebaruSan mentioned this pull request Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN Registry Issues affecting the registry Relationships Issues affecting depends, recommends, etc. Tests Issues affecting the internal tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants