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

rose-arch: Raise error for duplicate targets. #2138

Merged
merged 2 commits into from
Jan 25, 2018

Conversation

oliver-sanders
Copy link
Member

It is possible to pass rose arch a pair of duplicate targets using the [arch:(target)] syntax e.g:

[arch:foo]

[arch:(foo)]

This PR catches the configuration error before it surfaces from sqlite to provide a more helpful error.

@oliver-sanders oliver-sanders added this to the soon milestone Jan 12, 2018
@oliver-sanders oliver-sanders self-assigned this Jan 12, 2018

# Don't permit duplicate targets.
if s_key_tail in s_key_tails:
raise Exception('%s: Duplicate target "%s".' % (
Copy link
Member

Choose a reason for hiding this comment

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

Recommend that you raise a ConfigValueError.

@matthewrmshin
Copy link
Member

(Travis CI failure safe to ignore.)

@matthewrmshin
Copy link
Member

@sadielbartholomew please sanity check.

Copy link
Contributor

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

Good spot & solution! PR fix makes sense & works, though did notice a docstring typo.

@@ -39,6 39,13 @@
from time import gmtime, strftime, time


class RoseArchDuplicateError(ConfigValueError):

"""An exception raised if dupluicate archive targets are provided."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: 'dupluicate'

Copy link
Contributor

Choose a reason for hiding this comment

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

Merging and amending typo quickly with a separate PR, as advised by Matt.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

@sadielbartholomew sadielbartholomew merged commit 3f2d8a5 into metomi:master Jan 25, 2018
sadielbartholomew added a commit to sadielbartholomew/rose that referenced this pull request Jan 25, 2018
@matthewrmshin matthewrmshin modified the milestones: soon, next-release Jan 25, 2018
matthewrmshin added a commit that referenced this pull request Jan 25, 2018
@oliver-sanders oliver-sanders deleted the rose-arch branch July 30, 2018 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants