-
Notifications
You must be signed in to change notification settings - Fork 135
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
Use default config file if exists #68
base: main
Are you sure you want to change the base?
Conversation
@@ -33,7 +33,7 @@ with the filename of the changed file. (The symbol may be changed with the | |||
OPTIONS are given below: | |||
--all=false: | |||
Include normally ignored files (VCS and editor special files). | |||
-c, --config="": | |||
-c, --config="reflex.conf": |
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.
This isn't the output of running reflex -h
, is it?
You should update the -c
documentation string to mention reflex.conf
, particularly because it's not a normal flag default value.
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.
I would much appreciate some advice on how best to do that.
We can edit the help string to append the phrase Default: reflex.conf
at the end, so it appears like so:
-c, --config="":
A configuration file that describes how to run reflex
(or '-' to read the configuration from stdin). Default: reflex.conf
But that's not how pflag prefers to indicate default arg values, instead this feels better:
-c, --config="reflex.conf":
A configuration file that describes how to run reflex
(or '-' to read the configuration from stdin).
However, unless I'm missing something, the 2nd method can only be done through the value
arg of pflag.StringVarP
call, which I don't want to do because there's a special case, where -c
is not passed, but reflex.conf
doesn't exist either. It's not simply that "default value for -c is reflex.conf" but rather "default value for -c is reflex.conf if the file exists in current working dir", and I'm at loss as to how best to handle that third case.
I'm leaning towards a spin on the first method above mentioned, like so:
-c, --config="":
A configuration file that describes how to run reflex
(or '-' to read the configuration from stdin).
Default: reflex.conf – if it exists in current working directory.
Bump for visibility. Question above. |
@cespare can you review please? Its been so long tho. |
Fixes #41
I was going through my issues list and saw that I had opened a feature request (2.5 years ago, sorry) for a "rc file" in the current directory, such that if it exists, it is automatically used if
--config
flag is not set. I believe we settled on a straightforward name for the config file:reflex.conf
. This PR implements that in a very simple, straightforward way:Before the config flag is processed, we check if it's empty and if
reflex.conf
exists. If it does, the flag is quietly set to that value before rest of the flag processing takes place. I also modified the wording of an error to indicate that the flag validation behavior is the same whether--config
flag is specified or assumed (to be=reflex.conf
).I tested this by building the reflex binary and running it in a directory with and without a
reflex.conf
file and it seems to meet the expectations in this rudimentary, manual test. I'm not sure how to add an automated test for this. Contents ofmain()
do not seem to be autotested anyway.I should also update the documentation before this is ready for review.
Edit: Also, prefer not to add my name to authors list. This is extremely trivial and I don't feel that I deserve the credit.