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

Remove unnecessary parameter in Configuration methods #2608

Merged
merged 6 commits into from
Dec 12, 2018

Conversation

DasSkelett
Copy link
Member

defaultRepo

Configuration.LoadOrCreateConfiguration() gets passed a defaultRepo parameter.
This is a relic of the early days of CKAN it seems, when the repositories were saved in the GUI configuration, but this got moved to the registry in 536637f.
Since then the parameter sits there unused.

path

Also SaveConfiguration() gets passed a path parameter, which is redundant since there is a specific path variable in the Configuration class. It's a bit dangerous too, because it could lead to a difference between the real save path and the one set in the configuration.
Now SaveConfiguration() takes the value out of the according object of Configuration, which happened in Save() before.

Add SpaceAfterMethodCallName to code formatting guideline

I added $2.SpaceAfterMethodCallName = False to the solution file, because MonoDevelop always inserted a space between method calls an round brackets.
Don't know if Visual Studio is okay with this, but I think in the worst case VS auto-removes the line again.
I encountered a strange Github display bug with the indentation in CKAN.sln, hence the 2 additional commits.

DasSkelett and others added 4 commits December 8, 2018 16:06
Remove unnecessary repository parameter in Configuration.LoadOrCreateConfiguration()
Remove unnecessary passing of path in SaveConfiguration()
@DasSkelett
Copy link
Member Author

As I wrote this PR I was wondering if SaveConfiguration() shouldn't be private instead of public, this would force Save() to be called from outside, which I understood is the goal of having this separate function.

@HebaruSan
Copy link
Member

My rule of thumb: If you can make something private without breaking anything, do it.

@DasSkelett
Copy link
Member Author

That's a great rule of thumb :P

Make SaveConfiguration() and LoadConfiguration() privat.
This needed to change two Tests.
@DasSkelett
Copy link
Member Author

DasSkelett commented Dec 8, 2018

Okay, made it private, also LoadConfiguration() (because we have LoadOrCreateConfiguration()). This needed changing two tests to call LoadOrCreateConfiguration() instead of LoadConfiguration(), but that doesn't affect their result.

@HebaruSan HebaruSan added GUI Issues affecting the interactive GUI Pull request labels Dec 10, 2018
@politas politas merged commit 327b713 into KSP-CKAN:master Dec 12, 2018
@DasSkelett
Copy link
Member Author

That build fail was not my fault!

@HebaruSan
Copy link
Member

Build fail? What build fail? 😉

@DasSkelett DasSkelett deleted the removeOldParam branch December 12, 2018 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Issues affecting the interactive GUI Pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants