Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

zrhmn
Copy link

@zrhmn zrhmn commented Dec 3, 2019

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 of main() 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.

@@ -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":
Copy link
Owner

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.

Copy link
Author

@zrhmn zrhmn Dec 20, 2019

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.

@zrhmn
Copy link
Author

zrhmn commented Feb 17, 2020

Bump for visibility. Question above.

@duraki
Copy link

duraki commented Jul 22, 2023

@cespare can you review please? Its been so long tho.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use a default config filename
3 participants