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

Add a simple ZP name check #417

Closed
wants to merge 1 commit into from
Closed

Add a simple ZP name check #417

wants to merge 1 commit into from

Conversation

vikt0rs
Copy link

@vikt0rs vikt0rs commented Feb 20, 2017

Added a check, that ZP name starts with "ZenPacks." to avoid typos and
errors like #415

Added a check, that ZP name starts with "ZenPacks." to avoid typos and
errors like #415
@vikt0rs vikt0rs requested a review from cluther February 20, 2017 15:32
sys.exit(
"`{}` is a wrong name for ZenPack - zenpack name must starts "
"with `ZenPacks`. Please, check ZP name.".format(zenpack_name)
)
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest changing message to something like this:
"ZenPack name is incorrect, it does not match ZenPack naming requirenments. Please see documentation: https://zenpack-sdk.zenoss.com/en/stable/managing-zenpacks-4.html#naming-a-zenpack"

Copy link
Author

@vikt0rs vikt0rs Feb 21, 2017

Choose a reason for hiding this comment

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

I think it's better to explain the error in place - not provide some external link to some "magic" ZenPack naming requirements.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it's a good idea to perform this check and also to provide direct feedback. The main issue with this PR, though, is that the monolithic zenpacklib.py file is now vestigial and is not the appropriate place for the change.

I took your suggestion and created a similar change here:
#420
and also added a check for the length of the ZenPack namespace (since that might go wrong as well).

Copy link
Author

Choose a reason for hiding this comment

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

ok, I'll close this PR in favour of #420

@vikt0rs vikt0rs closed this Feb 23, 2017
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.

3 participants