Skip to content

Conversation

@fschrader1992
Copy link
Collaborator

This PR

  • adds a prototype for property values validation
  • uses the list given in formats to check for mandatory attributes for docs, sections and properties during validation
  • adds the possibility to validate standalone sections and properties
  • adds validation for sections and properties on initialization
  • adds respective tests

@coveralls
Copy link

coveralls commented Mar 6, 2020

Coverage Status

Coverage increased (+0.2%) to 75.442% when pulling 329588a on fschrader1992:master into 5cdbce4 on G-Node:master.

validate(prop)

for err in validate(prop).errors:
assert("error" not in err.rank)
Copy link
Member

Choose a reason for hiding this comment

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

Some tests with invalid objects that check if the correct errors are produced should also be added.

More useful would be to test the new functionality. This PR introduces validation on object creation, so the new functionality should be tested: Are the appropriate error messages printed when an object with issues is created? Since we just print the errors to stdout when this happens and the errors aren't stored anywhere after that, it might be necessary to capture stdout in the tests and check if the expected output was printed.

@fschrader1992
Copy link
Collaborator Author

fschrader1992 commented Mar 17, 2020

Checks for Code Quality:

  • Pylint
  • Rebase from GNODE Master
  • Repeat Local Tests

Copy link
Contributor

@mpsonntag mpsonntag left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! One additional small change and one comment (WARNING->ERROR) has not been resolved yet.

@fschrader1992
Copy link
Collaborator Author

Checks for Code Quality:

  • Pylint
  • Rebase from GNODE Master
  • Repeat Local Tests Python 3.8
  • Repeat Local Tests Python 2.7

@achilleas-k
Copy link
Member

Approved from me, though I'd like to see a few more tests for the validation. Will open an issue as follow-up to this.

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.

4 participants