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

Boolean config options #88

Merged
merged 6 commits into from
Mar 6, 2017
Merged

Boolean config options #88

merged 6 commits into from
Mar 6, 2017

Conversation

ata2001
Copy link
Contributor

@ata2001 ata2001 commented Mar 5, 2017

No description provided.

@ata2001 ata2001 mentioned this pull request Mar 5, 2017
@@ -21,7 +21,7 @@ def test_episode_already_exists_raise_exception(self):
existing_path = os.path.join(self.files, filename)
with open(existing_path, 'w'):
with raises(PathExistsException):
self.tv.rename(fn, existing_path)
self.tv.rename(fn, existing_path, symlink=False)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than update this test can you make the symlink arg default to False in the rename method please. That will better signify it's an optional parameter to the method.

@click.option('--symlink', is_flag=True, help="Create symbolic links instead of moving the files. Requires '--rename-dir'.") # noqa
@click.option('-t', '--the', is_flag=True, help="Set the position of 'The' in a show's name to the end of the show name") # noqa
@click.option('--symlink', is_flag=True, default=None, help="Create symbolic links instead of moving the files. Requires '--rename-dir'.") # noqa
@click.option('-t', '--the', is_flag=True, default=None, help="Set the position of 'The' in a show's name to the end of the show name") # noqa
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you expand on why these cli option changes were necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because otherwise the default value from click will override the config, even if the user didn't call that option. Check my comment on the issue #87!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, I missed that comment somehow! I'm happy with this solution 👍

@ata2001 ata2001 closed this Mar 6, 2017
@ata2001 ata2001 reopened this Mar 6, 2017
@ata2001
Copy link
Contributor Author

ata2001 commented Mar 6, 2017

Sorry, I accidentally clicked close on my touchscreen.

@ata2001
Copy link
Contributor Author

ata2001 commented Mar 6, 2017

Thanks for approving!

Sorry, but I don't really know, what does that mean. Do I need to do something right now? Or you'll just merge it later?

@ghickman ghickman merged commit eac4910 into ghickman:master Mar 6, 2017
@ghickman
Copy link
Owner

ghickman commented Mar 6, 2017

@ata2001 – just that I approved the changes you made based on my previous feedback.

Thanks again for these changes, you've been a real powerhouse, it's really appreciated :)

@ghickman
Copy link
Owner

ghickman commented Mar 6, 2017

Released in 4.3.0 on the PyPI

@ata2001 ata2001 deleted the config branch March 6, 2017 14:02
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.

2 participants