Skip to content
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

yaml validation #25

Merged
merged 3 commits into from
Oct 20, 2023
Merged

yaml validation #25

merged 3 commits into from
Oct 20, 2023

Conversation

zoldello
Copy link
Collaborator

@zoldello zoldello commented Aug 2, 2023

No description provided.

Copy link
Collaborator Author

@zoldello zoldello Aug 2, 2023

Choose a reason for hiding this comment

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

This file is shared with yaml creator. I tried using git subtree to centralize the access. But had issues with github actions on this app and react with the yaml creator. It would was a time-consuming battle on two fronts. So, I scrapped the idea and copied the file. In the future, I may create a new repo with only validation files. That should not cause github action issues or react issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I needed a set up where Unit test (meant to be fast and ran constantly) could easily be ran separate from other test (like integration test that are slow and meant to run on occasions like before push.) So, I created a folder. An option would of had been to add unit-test marker. But it felt jacky and brittle as it fails if one person does not follow convention.

Copy link
Collaborator Author

@zoldello zoldello Aug 2, 2023

Choose a reason for hiding this comment

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

The unit test for convert_yaml are failing and need to be modified as task_epochs associated_files and associated_video_files is an integer rather than an array. The unit test are failing due to to this and fixing them are beyond this PR scope.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did the yaml generator recently change what it produces for those (in the past month or so)? The yaml it's testing with comes from the yaml generator, so if this is the format people will have when they use the converter then should the validator accept that?

Or am I misunderstanding the issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It has been the way it has been for like two of three months. task.task_epochs is an array and associated_files.task_epochs and associated_video_files.task_epochs are integers.

@zoldello zoldello marked this pull request as ready for review August 2, 2023 19:34
@zoldello
Copy link
Collaborator Author

zoldello commented Aug 2, 2023

Github action is and fixing it is beyond the scope of this ticket. I different ticket will address it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in github action, we run -

pytest --doctest-modules -v --pyargs src/spikegadgets_to_nwb

That does not like two test with the same name regardless of being in different location. So, I added "_it" to this.

@edeno
Copy link
Collaborator

edeno commented Aug 31, 2023

@zoldello where are we on this?

@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (19c41e6) 91.25% compared to head (03de7ed) 91.63%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   91.25%   91.63%   +0.38%     
==========================================
  Files          20       24       +4     
  Lines        2058     2177     +119     
==========================================
+ Hits         1878     1995     +117     
- Misses        180      182       +2     
Files Coverage Δ
src/spikegadgets_to_nwb/metadata_validation.py 100.00% <100.00%> (ø)
...s/integration-tests/test_metadata_validation_it.py 100.00% <100.00%> (ø)
..._nwb/tests/test_data/test_metadata_dict_samples.py 100.00% <100.00%> (ø)
...kegadgets_to_nwb/tests/test_metadata_validation.py 100.00% <100.00%> (ø)
src/spikegadgets_to_nwb/convert_yaml.py 94.91% <66.66%> (-1.52%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zoldello
Copy link
Collaborator Author

@samuelbray32 @edeno rebased and test passed.

@samuelbray32 samuelbray32 merged commit ade9bfd into main Oct 20, 2023
10 checks passed
@edeno edeno deleted the yaml-validation branch November 7, 2023 01:03
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.

3 participants