Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork847
Add --config-file command line parameter#917
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Add --config-file command line parameter#917
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ff4e1b1 to52b9294CompareI think I'm done with this. Comments are appreciated. |
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.
@anakinj Thanks for the thorough Pull Request!
I made inline comments, and you can process them as and when you wish to, let me know when you want me to look again, and I'll get back to you.
| let(:argv) { [] } | ||
| before do | ||
| # Calling abort will abort the test run, allow calls to abort to not accidentaly get positive falses |
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.
Good context in this comment!
spec/unit/parser_spec.rb Outdated
| end | ||
| end | ||
| context "when --config-file is overriden to something that is not there" do |
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.
| context"when --config-file isoverriden to something that is not there"do | |
| context"when --config-file isoverridden to something that is not there"do |
spec/unit/parser_spec.rb Outdated
| end | ||
| end | ||
| context "when option with incorrect type is given" do |
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.
| context"when option with incorrect type is given"do | |
| context"whenanoption with incorrect type is given"do |
Uh oh!
There was an error while loading.Please reload this page.
| @@ -22,29 +44,22 @@ module GitHubChangelogGenerator | |||
| # | |||
| class ParserFile | |||
| # @param options [Hash] options to be configured from file contents | |||
| # @param file [nil,IO] configuration file handle, defaults to opening `.github_changelog_generator` | |||
| def initialize(options, file = open_settings_file) | |||
| # @param io [nil,IO] configuration file handle | |||
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.
| # @param io [nil,IO] configuration file handle | |
| # @param io [nil,IO] configuration file handle |
| class FileParserChooser | ||
| def initialize(options, config_file) | ||
| @options = options | ||
| @config_file = config_file |
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.
Minor: Would anything become easier or cooler ifPathname(config_file) was used here? I'm a big fan of Pathname.https://rubyapi.org/3.0/o/pathname
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.
It would. Great tip, will start using Pathname even more in the future !
| argv_parser.parse!(argv) # parse first to get config_file | ||
| FileParserChooser.new(options, options[:config_file]).parse! | ||
| argv_parser.parse!(argv) # parse again to override whatever was read from the config file |
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.
Minor: This all seems alright, and perhaps it doesn't need to be any more of a pipeline than now.
But I was thinking "what if we add more ways to input configuration? Would they have the styleXyzParser.new(options, options[:xyz_special_option_setting]).parse! too?
This comment is prefixed is "Minor:" which means "you can read it, and you absolutely don'thave to act or react to it".
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.
Do you mean to make the parameters even more generic to the parsers like:FileParserChooser.new(options).parse!
Then the parser can choose what/if it will do and the caller don't really need to care about special parameters.
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.
Took some inspiration from this comments and changed the parsing totally from the first proposed version.
| @@ -202,6 +189,9 @@ def self.setup_parser(options) | |||
| opts.on("--cache-file [CACHE-FILE]", "Filename to use for cache. Default is github-changelog-http-cache in a temporary directory.") do |cache_file| | |||
| options[:cache_file] = cache_file | |||
| end | |||
| opts.on("--config-file [CONFIG-FILE]", "Path to configuration file. Default is .github_changelog_generator.") do |config_file| | |||
| options[:config_file] = config_file | |||
| end | |||
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.
Thanks for adding this!
#945 now registered the issue (which you didn't introduce, mind you).
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.
Good catch. The surroundings impact how you do things :) Will adjust.
| options = default_options | ||
| ParserFile.new(options).parse! | ||
| class ArgvParser |
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.
In order to make the file layout more regular, could this class be in its own file?
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.
It should be yes. I'll adjust
Thanks@olleolleolle for the super-positive-and-nice review. I will take a look at the suggestions/comments when time allows. |
4508d6d toa41a853Comparea41a853 toff552d5CompareGreat comments. I did some adjustments and it turned out pretty nice I think. |
| puts opts | ||
| exit | ||
| end | ||
| end |
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.
When we have merged this, or earlier, we can figure out a way to not repeat (and need to maintain 2 copies of) these opts.on invocations.
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.
Not totally sure I follow. But where is the copy?
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.
Ah, it was only me being momentarily confused, we're already at that place. Perfect!
| ArgvParser, # Parse arguments first to get initial options populated | ||
| FileParserChooser, # Then parse possible configuration files | ||
| ArgvParser # Lastly parse arguments again to keep the given arguments the strongest | ||
| ].freeze |
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.
Nicely documented!
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.
Alright! We have gotten somewhere. Many small improvements.
Do you feel ready to merge?
Im done for now, so yeah. |
@anakinj Many thanks for taking the time to build & to revisit! |
This addresses#756
It's still a little WIP but comments are appreciated at this point, probably going to do some refactorings.
Was thinking we could support YAML configuration files also, should be pretty straightforward to implement.