-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add --no-confirm option #205
Conversation
@@ -30,6 +30,7 @@ | |||
-a --ascii-only Only allow ASCII chars (北京 (capital of china) -> bei-jing-capital-of-china) | |||
-k --keep-spaces Retain whitespace in filenames | |||
-u --keep-upper Retain uppercase letters in filenames | |||
--no-confirm Override confirmation prompts. Use with caution. |
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.
That help text is quick & dirty. Could be elaborated / written more precicely what it is about. Suggestions welcome!
@@ -49,6 +49,7 @@ def __init__(self, template, directory, overwrite, embed_lyrics, grouping, embed | |||
self.keep_space = keep_space | |||
self.keep_upper = keep_upper | |||
self.debugging = debugging | |||
self.confirmation_skip = no_confirm |
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.
Renamed here. Is that bad/confusing?
@@ -58,7 +59,7 @@ def start(self, album: dict): | |||
if self.debugging: | |||
logging.basicConfig(level=logging.DEBUG) | |||
|
|||
if album['full'] is not True: | |||
if not album['full'] and not self.confirmation_skip: |
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.
The reason for "renaming" no_confirm to confirmation_skip was to have this line "read nicely". Also I changed is not True
to just not
, I think that's more pythonic and fits better with the new and longer statement.
I prefer readability so it all looks fine to me. |
Give me a bit to push a pypi build, after a Windows update last night my system would no longer boot, startup repair, DISM repair, restore, etc nothing would fix it so I had to go through my usual process of backing everything important up from one of my live linux drives and doing a fresh install. I may finish the small bit of code needed for #201 just to give me a reason to version bump before doing so. |
which was added to the upstream project in iheanyi/bandcamp-dl#205. Release via PyPI is pending.
Hi, please drop me a note when you find time to kick out a new release. There is no rush on this, I'll watch your repo for releases I guess. Maybe this is just a reminder to myself :-) Thanks anyway! I've opened a draft PR in the actual project that was the reason for submitting this new feature: doctorfree/MusicPlayerPlus#31 I'll set this ready to merge once the feature is released. |
Once I've got my logger util functions sorted I can push my last changes and make a release. I had noticed during my testing after adding config files that debugging had likely been broken for some time as using the option output absolutely nothing. So I'm now making a proper set of loggers with formatting, log files, and rotating logs. |
Any chance you could release this tiny feature without ypur logger improvements? |
Currently backing up my dev branch and hopping back into master to see if I can get an actual Pypi release pushed, I haven't done so since Pypi removed the legacy backend as I previously had issues authenticating. Ideally I get a build released sometime today that has everything currently in master. Edit: Current master released as 0.0.15 on Pypi |
This new option allows to skip the confirmation prompt when albums are incomplete and possibly can be used for future things that might include prompting the user for something.
I went with --no-confirm as the long-option name since --yes could be confused with the already existing -y short-option. Most probably this option will be used in scripts, thus I think a medium long option name like that is ok.