-
Notifications
You must be signed in to change notification settings - Fork 23
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
Minor refactorings to consolidate common patterns #94
Conversation
This cleans the surrounding code up a bit to make it easier to re-work.
rpmtree and resolve use essentially the same resolve structure so we can extract it out into a common place to avoid duplication.
cmd/resolve.go
Outdated
resolveCmd.Flags().StringArrayVarP(&resolvehelperopts.in, "input", "i", nil, "primary.xml of the repository") | ||
resolveCmd.Flags().StringVar(&resolvehelperopts.baseSystem, "basesystem", "fedora-release-container", "base system to use (e.g. fedora-release-server, centos-stream-release, ...)") | ||
resolveCmd.Flags().StringVarP(&resolvehelperopts.arch, "arch", "a", "x86_64", "target architecture") | ||
resolveCmd.Flags().BoolVarP(&resolvehelperopts.nobest, "nobest", "n", false, "allow picking versions which are not the newest") | ||
resolveCmd.Flags().StringArrayVarP(&resolveopts.repofiles, "repofile", "r", []string{"repo.yaml"}, "repository information file. Can be specified multiple times. Will be used by default if no explicit inputs are provided.") | ||
resolveCmd.Flags().StringArrayVar(&resolveopts.forceIgnoreRegex, "force-ignore-with-dependencies", []string{}, "Packages matching these regex patterns will not be installed. Allows force-removing unwanted dependencies. Be careful, this can lead to hidden missing dependencies.") | ||
resolveCmd.Flags().StringArrayVar(&resolveopts.onlyAllowRegex, "only-allow", []string{}, "Packages matching these regex patterns may be installed. Allows limiting the dependency scope. Be careful, this can lead to hidden missing dependencies.") | ||
resolveCmd.Flags().StringArrayVar(&resolvehelperopts.forceIgnoreRegex, "force-ignore-with-dependencies", []string{}, "Packages matching these regex patterns will not be installed. Allows force-removing unwanted dependencies. Be careful, this can lead to hidden missing dependencies.") | ||
resolveCmd.Flags().StringArrayVar(&resolvehelperopts.onlyAllowRegex, "only-allow", []string{}, "Packages matching these regex patterns may be installed. Allows limiting the dependency scope. Be careful, this can lead to hidden missing dependencies.") | ||
// deprecated options | ||
resolveCmd.Flags().StringVarP(&resolveopts.baseSystem, "fedora-base-system", "f", "fedora-release-container", "base system to use (e.g. fedora-release-server, centos-stream-release, ...)") | ||
resolveCmd.Flags().StringVarP(&resolvehelperopts.baseSystem, "fedora-base-system", "f", "fedora-release-container", "base system to use (e.g. fedora-release-server, centos-stream-release, ...)") |
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.
If we want to make the refactoring really effective then this common ones (except for line 51) should go into a public method in resolve_helper.go that takes resolveCmd as argument and that adds this flags when needed, similar to what we do with the cache flags
This change consolidates the commandline options for resolve() into a single place to avoid (1) missing some options, (2) repetitive boilerplate.
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.
Looks good to me
This change contains a few minor refactorings to consolidate a few common patterns and avoid having to make changes in multiple places while evolving the code base.