Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Validate config changes #552

Open
wants to merge 3 commits into
base: issue-511
Choose a base branch
from

Conversation

ptth222
Copy link

@ptth222 ptth222 commented Mar 15, 2024

This PR goes all the way back to the initial meeting we had to discuss adding a way to opt out of the configuration validations. I have implemented that here. A "no_config" parameter was added to both the JSON and Tab validation and propagated appropriately so configuration validation can be optional. There are also a few messaging changes and some slight fixes.

One issue I found during testing was that some of the rules in the Tab validate have lines like result = result and func(). This essentially makes it to where the func() won't run after 1 error or warning is found. This eliminates some warnings or errors from being repeated multiple times, but also hides when there are multiple different ones at the same time. "test_b_ii_s_3" in "isatab/validate/test_core.py" is a good example. There are 2 different columns with warnings, but before this change you only saw 1 of those warnings, but those 2 warnings are repeated for each row. I'm not sure what would be the preference for how to handle this. I thought that maybe before issuing the warning there could be a check to see if the exact same warning was already issued and not do it again, but implementing this in a robust way did not seem trivial. For now, I simply left it so that the warnings/errors are repeated and changed the tests to match that where necessary.

ptth222 added 3 commits March 14, 2024 15:16
Found a possible issue during testing. Some rules had lines like result = result and func(). This essentially makes it to where the func() won't run after 1 error or warning is found. This eliminates some from being repeated multiple times, but also hides when there are multiple different ones at the same time.  "test_b_ii_s_3" in isatab/validate/test_core.py is a good example. There are 2 different columns with warnings, but before this change you only saw 1 of those warnings, but those 2 warnings are repeated for each row.
@coveralls
Copy link

Coverage Status

coverage: 81.258% (+0.001%) from 81.257%
when pulling 52c376f on ptth222:validate-config-changes
into 7d5f19f on ISA-tools:issue-511.

@proccaserra
Copy link
Member

further discussions needed: considered to be a draft

main concern is the usability and confusion of adding a 'no config' parameter when a configuration is passed as well

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants