-
Notifications
You must be signed in to change notification settings - Fork 38
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
Changed missing required properties to error #532
Conversation
It looks like some of the tests are specifically looking for the messages to be warnings. If it is agreed that these should be errors I think those tests have to change. |
@ptth222 +1 that makes sense. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good but, as with several other related PR, I am now investigating impact on tests (old ones) and possibly adding new ones.
If a property is "required" it should be an error, no? I also changed the investigation config file so that Study Person Mid Initial and Investigation Person Mid Initial aren't required.
There were quite a few changes because warnings became errors in a lot of tests. isa_tab/validate/test_core.py test_b_ii_s_3: The old warnings were: [{'message': 'Protocol declared but not used', 'supplemental': "protocols declared in the file s_BII-S-3.txt are not used in any assay file: {'sequence analysis - standard procedure 7', 'reverse transcription - standard procedure 5'}", 'code': 1019}, {'message': 'DOI is not valid format', 'supplemental': 'Found 10.1371/journal.pone.0003042 in DOI field', 'code': 3002}, {'message': 'DOI is not valid format', 'supplemental': 'Found 10.1111/j.1462-2920.2008.01745.x in DOI field', 'code': 3002}, {'message': 'A required property is missing', 'supplemental': 'A property value in Investigation Title of investigation file at column 1 is required', 'code': 4003}, {'message': 'A required property is missing', 'supplemental': 'A property value in Investigation Description of investigation file at column 1 is required', 'code': 4003}, {'message': 'A required property is missing', 'supplemental': 'A property value in Study Person Mid Initials of investigation file at column 2 is required', 'code': 4003}, {'message': 'A required property is missing', 'supplemental': 'A property value in Study Person Mid Initials of investigation file at column 3 is required', 'code': 4003}, {'message': 'A required property is missing', 'supplemental': 'A property value in Study Person Mid Initials of investigation file at column 4 is required', 'code': 4003}, {'message': 'A required property is missing', 'supplemental': 'A property value in Study Person Mid Initials of investigation file at column 5 is required', 'code': 4003}, {'message': 'A required property is missing', 'supplemental': 'A property value in Study Person Mid Initials of investigation file at column 6 is required', 'code': 4003}, {'message': 'A required property is missing', 'supplemental': 'A property value in Study Person Mid Initials of investigation file at column 7 is required', 'code': 4003}, {'message': 'A value does not correspond to the correct data type', 'supplemental': "Invalid value 'WGS' for type 'list' of the field 'Parameter Value[library strategy]'", 'code': 4011}] The required property warnings became errors. The other tests in this same file that were changed all had similar circumstances. test_clients/test_mw2isa.py test_conversion_ms: Required property warnings became errors, so I added their code, '4003', to the test. validators/test_validate_test_data test_validate_testdata_sra_chromatin_mod_seq_isatab: The following warnings became errors, so the test for 0 errors was broken: [{'message': 'A required property is missing', 'supplemental': 'A property value in Study Publication DOI of investigation file at column 1 is required', 'code': 4003}, {'message': 'A required property is missing', 'supplemental': 'A property value in Study Publication Author List of investigation file at column 1 is required', 'code': 4003}, {'message': 'A required property is missing', 'supplemental': 'A property value in Study Publication Title of investigation file at column 1 is required', 'code': 4003}] I changed the test to allow for more errors, but that might not be the best solution. All of the changed tests in this file had the same circumstances. convert/test_mzml2isa.py: The following warnings becamse errors: [{'message': 'A required property is missing', 'supplemental': 'A property value in Investigation Title of investigation file at column 1 is required', 'code': 4003}, {'message': 'A required property is missing', 'supplemental': 'A property value in Investigation Description of investigation file at column 1 is required', 'code': 4003}, {'message': 'A required property is missing', 'supplemental': 'A property value in Study Title of investigation file at column 1 is required', 'code': 4003}, {'message': 'A required property is missing', 'supplemental': 'A property value in Study Description of investigation file at column 1 is required', 'code': 4003}] This might suggest that there may be a problem with the converter if these values aren't being added.
a40a2d5
to
07ca3fa
Compare
Adding my last commit message here: There were quite a few changes because warnings became errors in a lot of tests. isa_tab/validate/test_core.py test_b_ii_s_3:
The required property warnings became errors. The other tests in this same file that were changed all had similar circumstances. test_clients/test_mw2isa.py test_conversion_ms: validators/test_validate_test_data test_validate_testdata_sra_chromatin_mod_seq_isatab:
I changed the test to allow for more errors, but that might not be the best solution. All of the changed tests in this file had the same circumstances. convert/test_mzml2isa.py:
This might suggest that there may be a problem with the converter if these values aren't being added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor merge conflicts to resolve.
The lines from the new commit need to be accepted. The tests had to change since this code changed a warning to an error. |
The DOI fix reduced 2 more warnings. Somehow that fix wasn't in this branch after rebasing.
Turned out neither of the lines worked. The DOI fix wasn't merged in yet I think. That fix reduced the warnings by 2, so I fixed the tests appropriately. |
closing but changes integrated via another PR . Thank you @ptth222 |
If a property is "required" it should be an error, no?
I also changed the investigation config file so that Study Person Mid Initial and Investigation Person Mid Initial aren't required. If this shouldn't be, feel free to put it back. It just seems odd to me to require a middle initial.