Skip to content

Conversation

@itowlson
Copy link
Collaborator

@itowlson itowlson commented Dec 4, 2024

Fixes #2946

Copy link
Contributor

@vdice vdice left a comment

Choose a reason for hiding this comment

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

I'm still confused on how the previous logic incorrectly determined 0.9.0 to be greater than 0.10.0... in any case, I wonder if now might be a good time to add a few unit tests here? (Or do we already have some?)

@itowlson
Copy link
Collaborator Author

itowlson commented Dec 4, 2024

@vdice The previous logic used lexical comparison of the version strings. 0=0, .=., but 9>1. (The PluginManifest::version field is a String not a Version, I assume because it is a serialisation type and needs to be forgiving of strings like "canary".)

There's a lot of places I would like to add unit tests for the plugin manager, but it requires some significant refactoring to do that. For example, the function I change here doesn't just validate the installability of the plugin, it also reads from disk and prints to the terminal. So testing it becomes a pain. I can pull out the bit that checks if this is a downgrade and test that function, but at that point we're barely above the level of testing if semver::Version implements Ord correctly. So: I agree: this does need a test; but I am not sure how to write a useful one in the current code structure.

Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this @itowlson

@itowlson itowlson merged commit 7ec2df8 into spinframework:main Dec 4, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plugin version number calculation seems off

4 participants