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.

Sign up for GitHub

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

Jump to bottom

Replace max KSP "9.9.9" with "any" #2432

Closed
wants to merge 2 commits into from
Closed

Replace max KSP "9.9.9" with "any" #2432

wants to merge 2 commits into from
+20 −1

Conversation

yalov
Copy link
Contributor

@yalov yalov commented Apr 27, 2018

#2420
KSP-CKAN/NetKAN#5356

CKAN GUI replace:

0.90    // unchanged
1.4     → 1.4.9
1.4.10  → 1.4.9
1.4.90  → 1.4.9
1.4.99  → 1.4.9
1.9.9   → any
9.99.99 → any

@yalov yalov changed the title Replace max KSP "9.9.9" with "any" and 1.4.99 with 1.4.9 Replace max KSP "9.9.9" with "any" and 1.4.99 with 1.4.9 in GUI Apr 27, 2018
@HebaruSan
Copy link
Member

I still don't understand why we wouldn't want to display accurately the values from the metadata.

@DasSkelett
Copy link
Member

DasSkelett commented Apr 27, 2018 •
edited
Loading

Seems like a good approach to me, but should we really consider a.b.9 as the highest possible a.b version number?
It might be unlikely, but technically it is possible that there is a double-digit version number in the future (a.b.cc). There already have been two-digit (0.90; 0.25; ...) releases.

HebaruSan
HebaruSan reviewed Apr 27, 2018
View reviewed changes
GUI/GUIMod.cs Outdated
@@ -115,7 +115,27 @@ public GUIMod(CkanModule mod, IRegistryQuerier registry, KspVersionCriteria curr
// KSP.
if (latest_available_for_any_ksp != null)
{
KSPCompatibility = KSPCompatibilityLong = registry.LatestCompatibleKSP(mod.identifier)?.ToString() ?? "any";
string str = registry.LatestCompatibleKSP(mod.identifier)?.ToString();
int major = registry.LatestCompatibleKSP(mod.identifier).Major;
Copy link
Member

Choose a reason for hiding this comment

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

Please don't call LatestCompatibleKSP more than once. It loops over all versions of the mod checking their compatibility, so the return value should go into a local variable.

HebaruSan
HebaruSan reviewed Apr 27, 2018
View reviewed changes
GUI/GUIMod.cs Outdated

if (str == null
|| (major + 1) % 10 == 0
|| (major == 1 && (minor + 1) % 10 == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Treating 1.9 as "any" is going to get us in trouble if Squad ever releases a 1.10 or 2.0 version.

@yalov
Copy link
Contributor Author

yalov commented Apr 27, 2018 •
edited
Loading

patch only twice reached .5 so .9 should be ok for the patch.

Now I start thinking about minor... it will reach 1.9.0 one day... so maybe 1.9.9 better leave unchanged

@HebaruSan ok, will fix that

HebaruSan
HebaruSan reviewed Apr 27, 2018
View reviewed changes
GUI/GUIMod.cs Outdated
&& minor != -1
&& (patch > 8 || patch == -1)
)
KSPCompatibility = major + "." + minor + ".9";
Copy link
Member

Choose a reason for hiding this comment

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

This would cause problems if Squad released a 1.4.10 version.

@HebaruSan
Copy link
Member

Regardless of whether we can convince ourselves now that .5 or .9 are safe, it raises the question again of why do this in the first place? It's hard coding a series of dubious assumptions about not just how many patches there will be but how Squad chooses to represent those versions in their numbering scheme. If any of those assumptions is violated in the future, then this code will need to be patched again with the new assumptions.

There's also value in being able to inspect the true values in the GUI, for example if you want to find these mods with weird versions for testing purposes. All things considered, I think we should leave this code as it is.

@yalov
remove patch changing, one LatestCompatibleKSP call

Verified

This commit was signed with the committer’s verified signature.
politas Myk
GPG key ID: 47938AAEAD85818F
Verified
Learn about vigilant mode
84c244d
@yalov
Copy link
Contributor Author

yalov commented Apr 27, 2018 •
edited
Loading

removed patch changing, but still don't want to have 99.99.99's or 1.99.99's in the column

@yalov yalov changed the title Replace max KSP "9.9.9" with "any" and 1.4.99 with 1.4.9 in GUI Replace max KSP "9.9.9" with "any" Apr 27, 2018
@yalov
Copy link
Contributor Author

yalov commented Apr 27, 2018

if Squad ever releases a 1.10 or 2.0 version

ok... they really can switch to "chrome" numeration.
Maybe you are right.

@yalov yalov closed this Apr 27, 2018
@pjf pjf removed the Pull request label Apr 27, 2018
@politas
Copy link
Member

politas commented Apr 28, 2018 •
edited
Loading

We have a table of actual ksp versions; maybe we can use that to programmatically limit max ksp compatibility to actual versions.

So, if a mod says max version is 1.3.99, but we know that the highest patch number in the 1.3 series is 2, then show max KSP as 1.3.2.
I would still say replace anything with a major version above 2 with "any"

@yalov
Copy link
Contributor Author

yalov commented Apr 28, 2018 •
edited
Loading

@politas, where is this table?

maybe find real_max_major, real_max_minor, and real_max patch (now they are 1, 90, 5 respectively), and use them to determine what should be converted to "any", and should be used x.y.9 or x.y.99

@DasSkelett
Copy link
Member

DasSkelett commented Apr 28, 2018 •
edited
Loading

https://github.com/KSP-CKAN/CKAN/blob/master/Core/builds.json
There.

Sorry, something went wrong.

@HebaruSan
Copy link
Member

And you can access that file via:

List<KspVersion> versions = new KspBuildMap(new Win32Registry()).KnownVersions;

Sorry, something went wrong.

@yalov yalov mentioned this pull request Apr 29, 2018
Merged
@HebaruSan HebaruSan mentioned this pull request Sep 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HebaruSan HebaruSan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yalov @HebaruSan @DasSkelett @politas @pjf