Skip to content

Move the parseConfig when run is called #50

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

Closed
wants to merge 1 commit into from
Closed

Move the parseConfig when run is called #50

wants to merge 1 commit into from

Conversation

dixonwille
Copy link

This is my proposed solution to #49

make test ran perfectly fine and the issue I was seeing did disappear (I was using delve to step through and see the config at different points during execution).

This should also mean that your other commands may be slightly faster, as it doesn't have to load the configuration file to execute.

Of course, if there is something I am overlooking then this PR and the Issue above can be closed.

@golangci
Copy link
Collaborator

golangci commented Jun 1, 2018

thank you for the pull request, but double parsing was made to give command line options higher priority than config file options. This PR breaks it. Maybe there is a better way? I will think about refactoring this code.

@dixonwille
Copy link
Author

@golangci I am almost positive spf13 thought about that. Let me push up another change tonight that will resolve that (I have an idea)?

@golangci
Copy link
Collaborator

golangci commented Jun 1, 2018

It would be great if you will find solution without double parsing

@dixonwille
Copy link
Author

So I did find a couple of solutions.

If I set my flags name to the following it maps to the proper configuration value and overrides what is in the config when I pass it in. This flag is too long to handle.

runCmd.Flags().StringSliceVar(&lsc.Depguard.Packages, "linters-settings.depguard.packages", nil,
		"Depguard: packages to add to the list")

But knowing the above lead me to three decisions.

  1. May one solution be to parse each sub-struct separately and then combine them in the end?
    • This would require multiple calls to viper.Unmarshal(&e.cfg) and unmarshaling is not cheap
  2. May another solution be to restructure the config.Config struct altogether.
    • This would be ideal in my opinion but that is not my decision to make. It would allow you to use viper to its greatest potential
  3. May another solution be to deal with the double parsing.
    • Considering it is adding to slice flags twice this can lead to bugs that may be difficult to track down.

I may attempt 1 here shortly and see how much of a hit it may be. What would you suggest?

@dixonwille
Copy link
Author

So option one will not work. As I would have to unmarshal the main struct for the config file and then the inner structs for the flags. But when I unmarshal the inner structs it seems to be using the flags default values and overiding the config file values even though the flag was never used...

@jirfag
Copy link
Contributor

jirfag commented Jun 2, 2018

#49 (comment)

@dixonwille
Copy link
Author

Close as #58 fixes this issue

@dixonwille dixonwille closed this Jun 4, 2018
@dixonwille dixonwille deleted the support/fix-double-flag-parsing branch June 4, 2018 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants