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

Configuration file #84

Merged
merged 6 commits into from Dec 14, 2013

Conversation

Projects
None yet
3 participants
@sebastinas
Contributor

sebastinas commented Nov 30, 2013

This adds a configuration file to isrcsubmit. It is located in a directory according to the XDG specification (the location on Windows is what googling suggests to use instead of $HOME/.config if $XDG_CONFIG_HOME is not set). Options given on the command line override settings from the configuration file.

doc/conf.py Outdated
[u'Johannes Dewender'], 1)
[u'Johannes Dewender'], 1),
('isrcsubmit-config.5', 'isrcsubmit-config', u'isrcsubmit Documentation',
[u'Johannes Dewender'], 5)

This comment has been minimized.

@Freso

Freso Nov 30, 2013

Contributor

I would say this line should be [u'Sebastian Ramacher'], 5) instead, unless @JonnyJD had a hand in writing the page that you haven't mentioned. :)

This comment has been minimized.

@JonnyJD

JonnyJD Dec 2, 2013

Owner

It isn't entirely clear who should be listed in AUTHORS.
http://man7.org/linux/man-pages/man7/man-pages.7.html :

                lists authors of the documentation or program.

(along with a paragraph telling that this isn't the place to give an exhaustive list of all authors)

Fixed by @sebastinas in b40e4cc (explicitely written who did what).

"backend": "",
"device": ""
}
})

This comment has been minimized.

@Freso

Freso Nov 30, 2013

Contributor

I kind of feel like the default values should be defined on their own somehow, instead of being defined in an argument in a function call. Even just setting it in its own variable and then using that in the function call would like neater to me, but I guess it is @JonnyJD's call. (But then, I might be inclined to make the configuration its own object with the three functions as methods and the default values here as a property.)

This comment has been minimized.

@JonnyJD

JonnyJD Dec 2, 2013

Owner

Hm, I am not sure if I understand the workflow here.
Without a config file or with a section or key missing in the config file, isrcsubmit should do what isrcsubmit does now (as defined per the constants defined at the top of the script and the defaults for the flags).

I don't fully understand why we want to do the intermediate step of creating a "default config file".
There is no config file written or anything.

I do understand that config.get("musicbrainz", "user") raises a NoSectionError when there is nothing set in a config file, so this is possibly a workaround? In that case we should use config.has_option() instead to test if an option is set in the config file at all and only use it to deviate from the defaults when it is.

So unless I overlook something obvious, we don't need this fill_config_with_default_values() at all.

This comment has been minimized.

@sebastinas

sebastinas Dec 2, 2013

Contributor

There are two reasons: 1) I had parser.add_options(..., default=config.get(...)) first so I had to deal with the NoSectionError and NoOptionError exception somehow. 2) The current code only honors options in the config file if they are set to some non-empty value. This would mean that all the parser.update_default calls need to be guarded with if config.has_option(...) and config.get(...) if fill_config_with_default_values were removed. Making sure that config.get never fails with NoSectionError or NoOptionError made those ifs just a bit shorter.

I don't have a strong opinion on that and do not care a lot. If you want it removed, that's possible.

This comment has been minimized.

@JonnyJD

JonnyJD Dec 2, 2013

Owner

Yes, if we want to allow empty values and take them as "use default", then we do need to test if an option is set and if it is non-empty.
I think that is still better than filling the config with default values.

Additionally, I am not sure if we should allow empty options and take them as defaults.
Failing with an error if an option is empty is also a possibility. Either way, we should test if the option is empty.

If you think it is a bit redundant to mention every option in two tests and when changing a default (3 times in 2 lines), then use a helper function that does both checks, given config, section and option as input. This is probably the way to go if we want to print an error message.

Right now I tend to not allow setting empty options (and printing an error if it is empty).
I don't really see the use of setting options to empty (rather than just to comment them out or removing them from a config file).

parser.add_option("--keyring", action="store_true", dest="keyring",
help="Use keyring if available.")
parser.add_option("--no-keyring", action="store_false", dest="keyring",
help="Disable keyring.")

This comment has been minimized.

@Freso

Freso Nov 30, 2013

Contributor

I feel like these --keyring options should be their own patch/pull request.

This comment has been minimized.

@sebastinas

sebastinas Dec 2, 2013

Contributor

One of them doesn't make sense without this pull request. I'll move that to a separate commit so that @JonnyJD can either merge everything or everything minus the --keyring/--no-keyring change.

This comment has been minimized.

@JonnyJD

JonnyJD Dec 2, 2013

Owner

--no-keyring would also work in a separate PR, but the whole thing is a follow-up to #83.
So keeping this in one PR is fine with me, even though just --no-keyring would need less review.

EDIT:
Of course having a config file has a lot more uses than just for the keyring setting, but that was the reason.

sebastinas added some commits Dec 2, 2013

Add config file handling
Signed-off-by: Sebastian Ramacher <sebastian+dev@ramacher.at>
Document configuration file
Signed-off-by: Sebastian Ramacher <sebastian+dev@ramacher.at>
Add keyring options
Add --keyring and --no-keyring to the command line options and keyring to
section [general] in the configuration file.

Signed-off-by: Sebastian Ramacher <sebastian+dev@ramacher.at>
@sebastinas

This comment has been minimized.

Contributor

sebastinas commented Dec 2, 2013

Rebased on the current master and unified the propagation of the values from the config file to OptionParser

sebastinas added some commits Dec 10, 2013

Get rid of fill_config_with_default_values
Signed-off-by: Sebastian Ramacher <sebastian+dev@ramacher.at>
Check if selected backend is valid
Signed-off-by: Sebastian Ramacher <sebastian+dev@ramacher.at>
Enable keyring per default
Signed-off-by: Sebastian Ramacher <sebastian+dev@ramacher.at>
@Freso

This comment has been minimized.

Contributor

Freso commented Dec 14, 2013

@JonnyJD Anything holding this back from being merged in? :) I don't see anything off with it at a glance, and Travis is happy.

@JonnyJD JonnyJD merged commit e6f38f0 into JonnyJD:master Dec 14, 2013

1 check passed

default The Travis CI build passed
Details
@JonnyJD

This comment has been minimized.

Owner

JonnyJD commented Dec 14, 2013

Oh, I didn't even realize there were changes.

Note that github doesn't notify when commits are added or changed in the PR.
The idea is probably that one can change/push the branch early, but add a comment only when the branch is done for another review.

I merged the branch.
I also added some fixes afterwards, if you guys want to have a look:
b4639cb...d8f0af7

Thanks @sebastinas for implementing all that stuff (config file, keyring).

@JonnyJD JonnyJD added the usability label May 5, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment