isa-api icon indicating copy to clipboard operation
isa-api copied to clipboard

Changed missing required properties to error

Open ptth222 opened this issue 1 year ago • 6 comments

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.

ptth222 avatar Feb 07 '24 02:02 ptth222

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 avatar Feb 07 '24 22:02 ptth222

@ptth222 +1 that makes sense.

proccaserra avatar Feb 07 '24 22:02 proccaserra

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 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.

ptth222 avatar Mar 11 '24 16:03 ptth222

Coverage Status

coverage: 81.255% (-0.002%) from 81.257% when pulling 1a8dab1722e63ba96ec7544a745e030281a76c40 on ptth222:inv-config-change into 7d5f19f9c7d2738849b2ac62e6d3c71603da8151 on ISA-tools:issue-511.

coveralls avatar Mar 11 '24 17:03 coveralls

The lines from the new commit need to be accepted. The tests had to change since this code changed a warning to an error.

ptth222 avatar Mar 12 '24 15:03 ptth222

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.

ptth222 avatar Mar 12 '24 19:03 ptth222

closing but changes integrated via another PR . Thank you @ptth222

proccaserra avatar Jul 19 '24 16:07 proccaserra