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 test for cuesubmit - Validators.py #588

Merged

Conversation

kalisp
Copy link
Contributor

@kalisp kalisp commented Dec 20, 2019

Link the Issue(s) this Pull Request is related to.
#396

Summarize your change.
Added first tests for Validators
Updated comment for one function in Validators.py based on test result

Validators.py didn't have any test coverage yet

Updated comment for one function in Validators.py
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 20, 2019

CLA Check
The committers are authorized under a signed CLA.

Copy link
Collaborator

@gregdenton gregdenton left a comment

Choose a reason for hiding this comment

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

LGTM. @bcipriano do you agree with the comment on whitespaces?

@@ -28,7 +28,7 @@ def matchLettersAndNumbersOnly(value):


def matchNoSpecialCharactersOnly(value):
"""Match strings containing letters, numbers, '.', '-', and '_'"""
"""Match strings containing letters, numbers, '.', '-','_', \t and \n"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great example of why tests are great. We probably don't want tabs and newlines in this regex, at least not for where it is currently used. I'll create a separate ticket for cleaning up the regex to remove the general whitespace character and be more explicit about single whitespaces.

@gregdenton
Copy link
Collaborator

@kalisp Thanks for the contribution! It's great to be increasing the test coverage.

@kalisp kalisp closed this Dec 24, 2019
@kalisp kalisp reopened this Dec 24, 2019
Copy link
Collaborator

@bcipriano bcipriano left a comment

Choose a reason for hiding this comment

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

Yep, agreed.

Thanks @kalisp!

@bcipriano bcipriano merged commit 1e96aa8 into AcademySoftwareFoundation:master Jan 7, 2020
@kalisp kalisp deleted the CueSubmitValidatorsTest branch January 9, 2020 17:54
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