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

ckan ksp fake/clone fixes #2642

Merged
merged 2 commits into from
Jan 10, 2019
Merged

Conversation

DasSkelett
Copy link
Member

@DasSkelett DasSkelett commented Jan 8, 2019

@HebaruSan discovered some issues with my new two functions. This is how they behave now:

Both functions:
Write human-readable messages if there are too few arguments provided, no error messages due to crashes.

ckan ksp fake:

  • Creates now all subdirs requested (see Add possibility to clone KSP installs and create dummy ones #2627 (comment)).
  • Added --set-default flag to make the new instance the default afterwards.
  • Also, if a user inputs a version number at least with major and minor, a selection dialog is raised to select a specific version (including build) known in the build map (else a message is raised to inform him to input a version with major and Minor at least).

This logic is now added as a new method of Versioning.KspVersion -> KspVersion RaiseVersionSelectionDialog(IUser).
Note that, despite the IUser being passed, only the cmdline User and the consoleUI User implement a RaiseSelectionDialog, so it won't work in the GUI without implementing a RaiseSelectionDialog.
Thanks @HebaruSan for the idea.

Fixes #2641
Fixes #2640
Fixes #2639
Fixes #2638
Fixes #2637

@DasSkelett
Copy link
Member Author

DasSkelett commented Jan 8, 2019

Now the new FindKnownVersion() (also added in #2627) is not used anymore.
I'm thinking of deleting it and maybe moving the version selection 'algorithm' from FakeNewKSPInstall()there, what do you suggest?

Edit: Did it - see PR comment.

@HebaruSan HebaruSan added Cmdline Issues affecting the command line Pull request labels Jan 8, 2019
Cmdline/Action/KSP.cs Outdated Show resolved Hide resolved
Cmdline/Action/KSP.cs Outdated Show resolved Hide resolved
@HebaruSan
Copy link
Member

HebaruSan commented Jan 9, 2019

Yes, I think moving the version selection logic makes sense so it could be shared among UIs. In the past this has been done by passing an IUser parameter. (Or perhaps your KSPManager already has an IUser object, probably should use that one if so.)

@HebaruSan
Copy link
Member

HebaruSan commented Jan 9, 2019

It would be really nice to have a --set-default option to ckan ksp fake, to make the new fake instance the default instance. This would be handy for the validation scripts.

@DasSkelett
Copy link
Member Author

DasSkelett commented Jan 9, 2019

Ooookay, I need some help understanding our Users/IUsers:
What are the virtual Display...() methods for?
Wouldn't it be easier to just override the Raise..() methods in each User class and thus derive them from the IUser interface instead of the NullUser class?

Edit: I see that the consoleUI does derive the User from the interface. Shouldn't this be the case for all Users (ConsoleUser and GUIUser)? At least that's what I think is the reason of the IUser interface, and the NullUser should be moved to in the Tests project and only be used there.

to `KspVersion.`RaiseVersionSelectionDialog` and fix that input 1 is not accepted.
Also add --set-default to `FakeNewInstall`.
@HebaruSan
Copy link
Member

Yeah, the structure is a bit messed up there, NullUser shouldn't really be inherited from, but I keep forgetting to go back and clean that up. If you want to fix it here, I would not complain. :)

@DasSkelett
Copy link
Member Author

DasSkelett commented Jan 9, 2019

Okay, will do that, but on a new branch and PR, to keep it separated.

@politas politas merged commit 8871a3a into KSP-CKAN:master Jan 10, 2019
politas added a commit that referenced this pull request Jan 10, 2019
@politas
Copy link
Member

politas commented Jan 10, 2019

Almost messed me up with that PR title. Bash ran the command when I tried to commit!

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 Pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants