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

[SOAR-12421] Name Validator #158

Merged
merged 3 commits into from
Jan 12, 2023
Merged

[SOAR-12421] Name Validator #158

merged 3 commits into from
Jan 12, 2023

Conversation

ablakley-r7
Copy link
Contributor

Proposed Changes

Description

Describe the proposed changes:

  • Validate plugin name

PR Requirements

Developers, verify you have completed the following items by checking them off:

  • Unit tests written for any new or updated code
  • Version bumped within setup.py
  • Changelog entry added

icon_validator/rules/plugin_validators/name_validator.py Outdated Show resolved Hide resolved
icon_validator/rules/plugin_validators/name_validator.py Outdated Show resolved Hide resolved
unit_test/test_validate_plugin.py Outdated Show resolved Hide resolved
unit_test/test_validate_plugin.py Outdated Show resolved Hide resolved
@ablakley-r7 ablakley-r7 added the Needs 2nd Reviewer PR needs a second reviewer label Jan 12, 2023
@dlaverty-r7 dlaverty-r7 merged commit a34ff2e into master Jan 12, 2023
@dlaverty-r7 dlaverty-r7 deleted the soar-12421_name_validator branch January 12, 2023 11:38
raise ValidationException("Name should not contain any whitespace characters.")
if not name.replace("_", "").isalnum():
raise ValidationException("Name should only contain alphanumeric values.")
if len(name.split("_")) > 7:
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure if this is a better change lemme know what you think

Suggested change
if len(name.split("_")) > 7:
len_name = len(name.split("_"))
if len_name > 7:

Copy link
Contributor

Choose a reason for hiding this comment

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

And obviously put the len_name variable at the top somewhere.

Comment on lines +16 to +17
if len(name.split("_")) > 7:
raise ValidationException(f"Name is too long, 7 words or less: contains {str(len(name.split('_')))}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if len(name.split("_")) > 7:
raise ValidationException(f"Name is too long, 7 words or less: contains {str(len(name.split('_')))}")
if len_name > 7:
raise ValidationException(f"Name is too long, 7 words or less: contains {str(len(name.split('_')))}")

raise ValidationException("Plugin name is missing.")
try:
NameValidator.validate_name(spec.spec_dictionary()["name"], plugin_name=True)
except Exception as error:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to specify the type of error here or is it better to handle all exceptions and assume they are related to plugin name not found

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs 2nd Reviewer PR needs a second reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants