Validator part 2 #421

achilleas-k · 2019-10-12T18:17:48Z

Did a bit of a cleanup and rewrite of the validator.

The format of the results is a nested dictionary of the form

results = {
    "errors": {
        nixobject: ["list of errors"]
    },
    "warnings": {
        nixobject: ["list of warnings"]
    }
}

The validator doesn't throw any exceptions (like it did before), but it doesn't catch any read exceptions either. I plan on adding this later so we can validate, for instance, a Block that's missing an ID. Currently, the library throws an exception if an ID is missing.
Error and warning strings are used from two static classes. This puts the strings in a single location, so they're easier to change and test.
Tests for every error and warning that doesn't break reading.

In a followup PR, I'll add a script so that the validator can be run from the command line.

- Renamed validate.py to validator.py - Validator module will simply be a collection of functions and not a class to instantiate with a file. There's no need to have an object.

Return the new style of results, which is a dictionary with two sub-dictionaries, one for errors and one for warnings.

Instead of returning a dictionary, each validation function (except check_file) returns a list of errors and a list of warnings for the provided object. None of the functions check objects recursively, but it may be necessary for Dimensions and Features.

Update tests as well

- Some conditionals were flipped - Empty string units in tags should be allowed

New helper function for checking Tag (and MultiTag) units against all the units in referenced DataArrays. Allows for the special case where both the Tag unit and the corresponding reference unit are empty strings.

Create fully valid file in test setUp() and create one error or warning per test.

Mostly the same as Tags, with differences in positions and extents checks.

Disable failing tests until they are rewritten

Can't test for the error now since the time can't be parsed by the lib.

lgtm-com · 2019-10-12T18:47:23Z

This pull request introduces 5 alerts when merging 7b3c338 into d493d01 - view on LGTM.com

new alerts:

3 for Module-level cyclic import
2 for Unused import

lgtm-com · 2019-10-12T19:20:01Z

This pull request introduces 3 alerts and fixes 3 when merging 307dccd into d493d01 - view on LGTM.com

new alerts:

3 for Module-level cyclic import

fixed alerts:

3 for Module-level cyclic import

lgtm-com · 2019-10-12T20:09:32Z

This pull request introduces 3 alerts and fixes 3 when merging 305d795 into d493d01 - view on LGTM.com

new alerts:

3 for Module-level cyclic import

fixed alerts:

3 for Module-level cyclic import

jgrewe · 2019-10-13T09:28:23Z

looks like we are caught in cycle here :)

jgrewe

Very nice work, the static classes make it super clear, imho

nixio/validator.py

achilleas-k · 2019-10-13T11:56:56Z

I don't think the detected import cycles are real cycles. Pycycle doesn't seem to think so. I guess the issue detected is that File imports validator which imports dimensions and dimensions haven't been imported by __init__.py yet.

EDIT: We could also drop the dimension type validation check since it's not really meaningful. The library initialises the Dimension object based on the dimension_type attribute when reading, so it's not like they can differ.

lgtm-com · 2019-10-13T12:15:03Z

This pull request introduces 3 alerts and fixes 3 when merging 5ed05de into d493d01 - view on LGTM.com

new alerts:

3 for Module-level cyclic import

fixed alerts:

3 for Module-level cyclic import

jgrewe · 2019-10-13T12:41:59Z

I don't think the detected import cycles are real cycles. Pycycle doesn't seem to think so. I guess the issue detected is that File imports validator which imports dimensions and dimensions haven't been imported by __init__.py yet.

EDIT: We could also drop the dimension type validation check since it's not really meaningful. The library initialises the Dimension object based on the dimension_type attribute when reading, so it's not like they can differ.

Could the dimension type be invalid? Not when the file was created via the library at least. I vote for skipping the check.

achilleas-k · 2019-10-13T12:59:26Z

Could the dimension type be invalid? Not when the file was created via the library at least. I vote for skipping the check.

It's the same as with other kinds of invalid states (like not having an ID). The instantiation of the object will throw an exception, so there's way to have a Dimension object that's has an invalid dimtype. I'll include it in the next version that will be able to catch exceptions and report them as errors.

The library initialises the Dimension object based on the dimension_type attribute when reading, so it's impossible for them to differ without the library throwing an exception on load.

lgtm-com · 2019-10-13T15:30:03Z

This pull request fixes 3 alerts when merging a7fd01c into d493d01 - view on LGTM.com

fixed alerts:

3 for Module-level cyclic import

achilleas-k added 24 commits October 9, 2019 17:01

[validator] Module functions instead of class

10454f0

- Renamed validate.py to validator.py - Validator module will simply be a collection of functions and not a class to instantiate with a file. There's no need to have an object.

[validator] New Block and Group check functions

3eb1c8f

Return the new style of results, which is a dictionary with two sub-dictionaries, one for errors and one for warnings.

[validator] Update block validation test

62f83ed

[validator] Clean up group validation and test

cb89a68

[validator] Check DataArrays and Dimensions

d716472

Update tests as well

[validator] Clean up imports

6e0ae8e

[validator] Simplify get_dim_units helper func

18604af

[validator] Remove old Validator class

424389d

[validator] Check Tags

fcb5c0f

[validator] Fix some logical errors

f1c1490

- Some conditionals were flipped - Empty string units in tags should be allowed

[validator] Cleaner (and correct) unit match checking

a622788

New helper function for checking Tag (and MultiTag) units against all the units in referenced DataArrays. Allows for the special case where both the Tag unit and the corresponding reference unit are empty strings.

[validator] Test rewrite

20d72eb

Create fully valid file in test setUp() and create one error or warning per test.

[validator] Check MultiTags

bae3228

Mostly the same as Tags, with differences in positions and extents checks.

[validator] Test MultiTags

369e459

Disable failing tests until they are rewritten

[validator] Static classes for error and warning strings

7ac491b

[validator] More tests (File and DataArray)

f2c703f

[validator] Allow creation date to be = 0

07381fb

Can't test for the error now since the time can't be parsed by the lib.

[validator] Check features and sources

6f50212

[validator] Check Sections and Properties

fdd2fb8

[validator] Add Sections and Properties to test file

87fa6a7

[validator] Tests for Section and Property

d81993a

[validator] Tests for Dimension errors and warnings

b6042c5

Delete unused debugging function in validator test

7b3c338

Delete unused code and import

307dccd

Fix cyclic import warning

305d795

jgrewe previously approved these changes Oct 13, 2019

View reviewed changes

nixio/validator.py Outdated Show resolved Hide resolved

[validator] Move closure to outermost valid scope

5ed05de

achilleas-k dismissed jgrewe’s stale review via 5ed05de October 13, 2019 11:53

[validator] Don't check dimension_type

a7fd01c

The library initialises the Dimension object based on the dimension_type attribute when reading, so it's impossible for them to differ without the library throwing an exception on load.

jgrewe approved these changes Oct 14, 2019

View reviewed changes

jgrewe merged commit ac0d807 into G-Node:master Oct 14, 2019

achilleas-k deleted the validator-part-2 branch October 14, 2019 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validator part 2 #421

Validator part 2 #421

achilleas-k commented Oct 12, 2019

lgtm-com bot commented Oct 12, 2019

lgtm-com bot commented Oct 12, 2019

lgtm-com bot commented Oct 12, 2019

jgrewe commented Oct 13, 2019

jgrewe left a comment

achilleas-k commented Oct 13, 2019 •

edited

Loading

lgtm-com bot commented Oct 13, 2019

jgrewe commented Oct 13, 2019

achilleas-k commented Oct 13, 2019

lgtm-com bot commented Oct 13, 2019

Validator part 2 #421

Validator part 2 #421

Conversation

achilleas-k commented Oct 12, 2019

lgtm-com bot commented Oct 12, 2019

lgtm-com bot commented Oct 12, 2019

lgtm-com bot commented Oct 12, 2019

jgrewe commented Oct 13, 2019

jgrewe left a comment

Choose a reason for hiding this comment

achilleas-k commented Oct 13, 2019 • edited Loading

lgtm-com bot commented Oct 13, 2019

jgrewe commented Oct 13, 2019

achilleas-k commented Oct 13, 2019

lgtm-com bot commented Oct 13, 2019

achilleas-k commented Oct 13, 2019 •

edited

Loading