-
Notifications
You must be signed in to change notification settings - Fork 91
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
read command parameters from config sections #240
Conversation
When this gets merged I suggest cutting a 0.6.1 release, since a fully working config file is definitely a worthy bugfix to push to users. Don't forget to bump the version number. |
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.
Minor change(s) to the README. Code changes LGTM at a glance. Some tests would be nice (that'll break before applying patch and succeed after the patch), just for additional robustness and to try and prevent regressions.
README.md
Outdated
when possible. | ||
The configuration file is stored in | ||
`$XDG_CONFIG_HOME/whipper/whipper.conf` or | ||
`$HOME/.config/whipper/whipper.conf` if `$XDG_CONFIG_HOME` is undefined. |
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 probably separate the two possibilities a bit stronger. Maybe a comma (,
) or (en‐)hyphen (–
) before the or
?
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.
As in, '"XDG...", or "HOME" if'?
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.
Yeah, exactly.
README.md
Outdated
- Drive section: `[drive:IDENTIFIER]`, one for each configured drive. All these values are probed by whipper and should not be edited by hand. | ||
- `defeats_cache`: whether this drive can defeat the audio cache | ||
- `read_offset`: the read offset of the drive | ||
```Python |
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.
Shouldn't this be INI
? There's nothing Python specific about the configuration file, just standard INI file syntax, no?
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 was trying to figure out what format would be appropriate and gave up after some searching through github's massive file-type highlighting database.
e6014e3
to
aa2de27
Compare
aa2de27
to
61afafc
Compare
Joe, please don't push changes to my personal development branches-- I'll rebase onto master while the PR is still open. I'm not even sure how you had the permissions to push to my personal repository... |
Sorry, did that with best intentions (to solve a merge conflict).
I haven't done anything fancy at all: just used GitHub's website. In this discussion I found a "resolve conflicts" button which, after being pressed, opened a file editor showing the file affected by the conflict: then I edited it to solve the conflict. Finally GitHub asked me to merge the aforementioned change (which I did; that went to your fork). To be frank that's something which left me surprised too. |
Yeah it's a reasonable thing to do in general, it just bugs me to have someone pushing to my personal repo (because the way I find out is by having a |
ACK. |
README.md
Outdated
Specification](http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html) | ||
and [ConfigParser](https://docs.python.org/2/library/configparser.html). | ||
|
||
The configuration file consists of newline-deliniated `[sections]` |
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 think this line has a typo: newline-deliniated
-> newline-delineated
.
Btw, re: committing to branches, you, @RecursiveForest, should see an option to disallow this—both when creating the PR to begin with, but also after the PR has been made. See https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/ for one way to toggle this repository/fork-wide. I couldn't find information about the per-PR option, but if you look for "Allow edits from maintainers." on this PR you should find a checkbox. There should be one at the time of PR creation too, but I can't remember for sure right now. |
Yeah there is at PR creation time. |
If it's completed, I'd like to merge it. 😉 |
I don't think any changes have been requested and both Freso and Merlijn have signed off, so I'd say go for it. |
fixes #99
Note that configuration sections now begin with
whipper
instead ofrip
to correspond to their command line invocation. e.g.,whipper cd rip
options go into section[whipper.cd.rip]
,whipper drive analyze
into[whipper.drive.analyze]
, etc.