KSP-ModularManagement / KSPe

Extensions and utilities for Kerbal Space Program
http://ksp.lisias.net/add-ons/KSPAPIExtensions
Other
11 stars 6 forks source link

KSPe.External.AddOnVersionChecker method `LoadFrom(string)` royally screwed the `LoadFrom(string, string =null)` #63

Closed Lisias closed 10 months ago

Lisias commented 1 year ago

A kind user realised I royally screwed the method mentioned on the tittle on TweakScale!

https://github.com/TweakScale/TweakScale/issues/318

Fix the mess!

Lisias commented 1 year ago

Fixed on commit https://github.com/net-lisias-ksp/KSPe/commit/9341f966bbcbfbdd92ccce6c2d005bce5a5ad06f .

Lisias commented 10 months ago

Reopening as I left his cake half baked.

https://github.com/net-lisias-ksp/KSPe/commit/9341f966bbcbfbdd92ccce6c2d005bce5a5ad06f

Lisias commented 10 months ago

Unless I left some more drunkness behind, commit https://github.com/net-lisias-ksp/KSPe/commit/023c023092bddbabae135d37cb88e2c859d8a60f finishes properly the job.

handw3rker commented 10 months ago

@Lisias Sorry to bother you on this issue, but I'm stupid and should turn on my brain before spewing nonsense. In my initial post at TweakScale/TweakScale#318 i was wrong, at least partial. In the error message i posted the problem was indicated the right way.

Could not find file "C:\Program Files (x86)\Steam\steamapps\common\Kerbal Space Program\TweakScaleCompanion".

SHAME ON ME!

Sure, this must come from System.IO.File.ReadAllText(pathname)!

Assuming IO.Hierarchy.GAMEDATA.Solve() returns the path to the addon directory (as the exception told us the right one) I think you have to append filename and extension instead of only the extension. So I propose to change

https://github.com/net-lisias-ksp/KSPe/blob/81900c2cd32e5009582f28b73d2a65699c90ee99/Source/KSPe.External/Util/AddOnVersionChecker.cs#L190-L196

to

return LoadVersionFromFile(
    SIO.Path.Combine(
        (null != vendor)
            ? IO.Hierarchy.GAMEDATA.Solve(vendor, addonName)
            : IO.Hierarchy.GAMEDATA.Solve(addonName)
        , addonName + ".version"
    )
);

Again, I'm sorry to led you down the wrong way since I appreciate your work on such hugh projects for the community... Hopefully I can be a little bit more helpful in future... I'm working on...

Lisias commented 10 months ago

@handw3rker :

OUKEY… Whatever both of us are drinking, we should stop! :D

Your analysis makes sense!

Yes, you are right - Solve returns a directory (if you get into the source code, you will notice I guarantee a trailing directory separator).

However… I wrote that code that way for a reason - I just can't remember why. There's a chance that I wrote it for an use case that was lost in time, or perhaps for an use case that I forgot about. I need to check this with more caution now.

Well, the thing was screwed before, and the thing is screwed right now - so no (new) harm done. To tell you the true, it's less screwed right now as we don't have a eternal recursive call anymore. It's a step ahead in a way or another.

Thank you very much for the heads up, and don't worry about the mistake - Kraken knows how many of these I do everyday, it only happens that most of the time I get them before shipping. :D

Things get a little hairy when I'm buried on Real Life™ and Day Job© tasks, what steals :P the time I usually spend testing things before shipping. Believe it or not, I'm on of these days - I had a bit hectic of a Seasons this year! :)

handw3rker commented 10 months ago

I feel the same way. Since my initial post on November I've played not 1 hour. Only fixing some minor problems on several addons. Normally i tried to fix it locally recompile it and test it, but KSPe is a "little bit" to complex... for me at this time.

Lisias commented 10 months ago

However… I wrote that code that way for a reason - I just can't remember why. There's a chance that I wrote it for an use case that was lost in time, or perhaps for an use case that I forgot about. I need to check this with more caution now.

In fact, I really did implemented that originally for an use case - the file system schema from TweakScale Companion "The Überpaket". But I had already screwed that use case before the problem detected by @handw3rker - it was on the backlog to fix that early last year, but with all that drama on CKAN, I just shrugged and gone take care of Real Life™ instead. And I ended up forgetting about.

Oh, well… Better late then never. Commit https://github.com/net-lisias-ksp/KSPe/commit/36bd8db207783c9b4438ab60ec32d1df6217f95e finished the job properly. I hope… :P