Adds a --config option to pass a path to a .pronto.yml config file #228

jeroenj · 2017-04-24T07:29:26Z

closes #226

jeroenj · 2017-04-24T07:29:46Z

@mmozuras I'd be happy to incorporate any feedback. :)

Not very happy with passing it as an ENV variable but I saw no easy/clean way to do it otherwise.

doomspork · 2017-04-24T22:08:01Z

lib/pronto/config_file.rb

- def initialize(path = '.pronto.yml')
- @path = path
+ def initialize
+ @path = if File.exist?(ENV['PRONTO_CONFIG'].to_s)


I think we should raise an error and not silently use the default:

def initialize @path = ENV.fetch('PRONTO_CONFIG', DEFAULT_FILE_PATH) raise "configuration file `#{@path}` missing" unless File.exists?(@path) end

Because of https://github.com/mmozuras/pronto/pull/228/files#diff-d06fbb141ccb6c55a35f4ed5668f6752R19 PRONTO_CONFIG will always have a default value. That mens that if no config file exists (which isn't unusual) it would raise an error. But I agree that if it's set to a different value and the file does not exist that it should raise an error.

I didn't think about Hash#fetch. I'll update the code. 👍

I've changed it including raising an error when the given path does not exist.

doomspork · 2017-04-24T22:10:06Z

spec/pronto/config_file_spec.rb

@@ -33,9 +33,41 @@ module Pronto
 context 'only global excludes in file' do
 before do
 File.should_receive(:exist?)
+ .and_return(false)


Should these File.should_receive(:exist?) calls specify their filename?

doomspork · 2017-04-25T15:07:14Z

lib/pronto/config_file.rb

- @path = path
+ def initialize
+ @path = ENV.fetch('PRONTO_CONFIG', DEFAULT_FILE_PATH)
+ return if @path == DEFAULT_FILE_PATH || File.exist?(@path)


Is this check really necessary? If the custom file is missing or the default is missing, we should still raise an error.

What is the difference between the suggested code and this:

def initialize @path = ENV.fetch('PRONTO_CONFIG', DEFAULT_FILE_PATH) raise "configuration file `#{@path}` missing" unless File.exists?(@path) end

I don't see a value in @path == DEFAULT_FILE_PATH, what is the goal? Shouldn't we raise an error even if the default is missing?

ENV['PRONTO_CONFIG'] will always be set because it's the default value of the --config option in Pronto::CLI.

An option could be to drop the default value for the --config option and keep the same logic in this code to fall back to .pronto.yml (like you're suggesting).

@jeroenj I understand it will be set with a value but we should still ensure that the value (the default .pronto.yml) is present. In the code you have we don't bother checking for .pronto.yml we just assume it's there. In the event that isn't, we won't catch it and instead an error will be raised elsewhere.

Since it will always be set it would mean that .pronto.yml would always be required if no custom file path is set. That means that every project should have such a config file.

It should only raise an error if a --config or PRONTO_CONFIG is used. In case it is not and pronto falls back to .pronto.yml (which is the default behavior) it should not raise an error if that file does not exist.

There's already a File.exist? check in the to_h method that you might be interested in... 😃

@nbekirov yeah. I think it should still be in there to check if the default file (.pronto.yml) exists...

@doomspork I've removed the default option for --config and changed the check to always raise an error if it (or the PRONTO_CONFIG env variable) is set. If not it continues and sets it to .pronto.yml which is handled in the File.exist? check @nbekirov is talking about.

nbekirov · 2017-04-26T07:19:45Z

lib/pronto/cli.rb

+ method_option :config,
+ type: :string,
+ default: '.pronto.yml',
+ desc: 'Uses the specified cofnig file.'


There's a typo in the word cofnig.

Cood catch. My hands tend to get out of sync. :(

Fixed. 👍

mmozuras · 2017-04-26T19:13:47Z

Not very happy with passing it as an ENV variable but I saw no easy/clean way to do it otherwise.

Let's try to think of alternatives before committing to passing values via ENV variables. This would require more refactoring, but maybe the whole Config class could be global, initialised only once?

jeroenj · 2017-04-27T06:42:48Z

Let's try to think of alternatives before committing to passing values via ENV variables. This would require more refactoring, but maybe the whole Config class could be global, initialised only once?

I think that would make sese. :) 👍

doomspork · 2017-07-15T19:45:45Z

What was the plan with this PR? Are we going to come up with a new approach or should this be closed for the time being?

jeroenj · 2017-07-15T20:57:08Z

Yes, I'm still planning on continuing to work on it but I had a lot on my plate the last few months which resulted in not enough time for this. I'm on holiday right now but I plan to pick this up again mid-August. Unless someone else picks in of course. :)

jeroenj · 2017-08-11T14:08:55Z

Let's try to think of alternatives before committing to passing values via ENV variables. This would require more refactoring, but maybe the whole Config class could be global, initialised only once?

@mmozuras I've made the Config a singleton object to make it global. There are still some pending tests but I've pushed the (working) code. Would you mind having a look at it already?

doomspork · 2018-06-12T21:32:38Z

ping @jeroenj — any progress here?

jeroenj · 2018-06-13T07:47:18Z

I didn't work on it anymore since then. I just (quickly) reread this. I think it would be best if @mmozuras or you gave feedback on the current implementation. Once we settle on an implementation I'll finish the tests so this can be merged. 👍

(I've been using this code in a fork since then actually.)

ashkulz · 2022-11-02T05:20:15Z

I'm closing this since #435 was merged, please reopen and fix merge conflicts if you still think this should be merged.

jeroenj force-pushed the config-file branch 2 times, most recently from a99b2b3 to 8e10708 Compare April 24, 2017 08:07

jeroenj changed the title ~~Adds a --config option to pass a path to awq .pronto.yml config file~~ Adds a --config option to pass a path to a .pronto.yml config file Apr 24, 2017

doomspork reviewed Apr 24, 2017

View reviewed changes

jeroenj force-pushed the config-file branch 4 times, most recently from 003199e to 3ba3604 Compare April 25, 2017 07:14

doomspork reviewed Apr 25, 2017

View reviewed changes

nbekirov reviewed Apr 26, 2017

View reviewed changes

jeroenj force-pushed the config-file branch from 3ba3604 to dc1caf4 Compare April 26, 2017 09:23

jeroenj added 3 commits August 11, 2017 10:45

Adds a --config option to pass a path to a .pronto.yml config file

ec40e76

Raise an error when the passed config file does not exist

0725d3e

Drops the default option for --config

a9eae2d

jeroenj force-pushed the config-file branch from dc1caf4 to a9eae2d Compare August 11, 2017 08:47

Refactors Config into a Singleton object

2341011

doomspork added enhancement and removed enhancement labels May 18, 2019

hosamaly mentioned this pull request Oct 21, 2022

Support specifying the config file path in an environment variable #435

Merged

ashkulz closed this Nov 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a --config option to pass a path to a .pronto.yml config file #228

Adds a --config option to pass a path to a .pronto.yml config file #228

jeroenj commented Apr 24, 2017

jeroenj commented Apr 24, 2017 •

edited

Loading

doomspork Apr 24, 2017

jeroenj Apr 25, 2017

jeroenj Apr 25, 2017

doomspork Apr 24, 2017

doomspork Apr 25, 2017 •

edited

Loading

jeroenj Apr 25, 2017

doomspork Apr 25, 2017

jeroenj Apr 25, 2017

nbekirov Apr 26, 2017

jeroenj Apr 26, 2017

nbekirov Apr 26, 2017

jeroenj Apr 26, 2017

mmozuras commented Apr 26, 2017

jeroenj commented Apr 27, 2017

doomspork commented Jul 15, 2017

jeroenj commented Jul 15, 2017

jeroenj commented Aug 11, 2017

doomspork commented Jun 12, 2018

jeroenj commented Jun 13, 2018

ashkulz commented Nov 2, 2022

Adds a --config option to pass a path to a .pronto.yml config file #228

Adds a --config option to pass a path to a .pronto.yml config file #228

Conversation

jeroenj commented Apr 24, 2017

jeroenj commented Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doomspork Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmozuras commented Apr 26, 2017

jeroenj commented Apr 27, 2017

doomspork commented Jul 15, 2017

jeroenj commented Jul 15, 2017

jeroenj commented Aug 11, 2017

doomspork commented Jun 12, 2018

jeroenj commented Jun 13, 2018

ashkulz commented Nov 2, 2022

jeroenj commented Apr 24, 2017 •

edited

Loading

doomspork Apr 25, 2017 •

edited

Loading