Add --config-file command line parameter #917
Conversation
ff4e1b1 to
52b9294
Compare
|
I think I'm done with this. Comments are appreciated. |
There was a problem hiding this comment.
@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.
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.
| context "when --config-file is overriden to something that is not there" do | |
| context "when --config-file is overridden 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.
| context "when option with incorrect type is given" do | |
| context "when an option with incorrect type is given" do |
| @@ -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.
| # @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.
Minor: Would anything become easier or cooler if Pathname(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.
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.
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 style XyzParser.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't have to act or react to it".
There was a problem hiding this comment.
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.
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.
Thanks for adding this!
#945 now registered the issue (which you didn't introduce, mind you).
There was a problem hiding this comment.
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.
In order to make the file layout more regular, could this class be in its own file?
There was a problem hiding this comment.
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 to
a41a853
Compare
a41a853 to
ff552d5
Compare
|
Great 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.
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.
Not totally sure I follow. But where is the copy?
There was a problem hiding this comment.
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 |
|
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.