-
Notifications
You must be signed in to change notification settings - Fork 230
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 for issue #1849 #1851
Fix for issue #1849 #1851
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything essentially looks good. But you didn't actually implement the change that you originally cited in #1849. Could you also make the correction to the toolbar wrapper?
@@ -55,6 +55,17 @@ public class KACWrapper | |||
|
|||
public static Boolean NeedUpgrade { get; private set; } | |||
|
|||
internal static Type GetType(string name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't really need to be internal
, doing so in the IRWrapper
makes a little sense because it is consistent with other wrappers, but it could simply be private for KACWrapper
. Not a big enough deal to require a change alone, but I'm going to request another change anyways so I thought I might as well comment.
OK, I hadn't seen the error for the toolbar from kOS and never expected that kOS supports blizzy's toolbar. I updated the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll double check operation later tonight, but looks good to go. I'll self assign and hopefully merge tonight.
In reviewing this PR (KSP-KOS#1851) I noticed that our source was old (old copyright dates) and didn't match the source at https://github.com/blizzy78/ksp_toolbar/blob/master/Wrapper/ToolbarWrapper.cs This updates the ToolbarWrapper.cs to match blizzy's current file. Most of it appears to be a formatting change, but I thought it would be good to staty current.
This fixes the errors I see, when ContractConfigurator is installed. #1849