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.

Sign up for GitHub

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

Jump to bottom

[ENH] Validator inheritance support #1120

Merged
merged 13 commits into from
Aug 2, 2022
Merged

[ENH] Validator inheritance support #1120

merged 13 commits into from
Aug 2, 2022

Conversation

TheChymera
Copy link
Collaborator

@TheChymera TheChymera commented Jun 8, 2022 •
edited
Loading

As discussed 2 weeks ago, the XS validator does not cover inheritance, a BIDS feature which may or may not cause more issues than it's worth.

Should work but needs more work, draft for now.

@codecov
Copy link

codecov bot commented Jun 8, 2022 •
edited
Loading

Codecov Report

Merging #1120 (32fb213) into master (f20986b) will increase coverage by 0.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1120      +/-   ##
==========================================
+ Coverage   91.60%   91.82%   +0.22%     
==========================================
  Files          10       10              
  Lines        1060     1089      +29     
==========================================
+ Hits          971     1000      +29     
  Misses         89       89
Impacted Files Coverage Δ
...schemacode/bidsschematools/tests/test_validator.py 100.00% <100.00%> (ø)
tools/schemacode/bidsschematools/validator.py 92.94% <100.00%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3e67f0...32fb213. Read the comment docs.

@TheChymera TheChymera changed the title [ENH] Draft inheritance support [ENH] Validator inheritance support Jun 9, 2022
@TheChymera TheChymera marked this pull request as ready for review June 9, 2022 09:38
@TheChymera TheChymera requested a review from tsalo as a code owner June 9, 2022 09:38
@TheChymera
Copy link
Collaborator Author

TheChymera commented Jun 9, 2022 •
edited
Loading

test failure is URL resolution, so should be good to review.
@effigies interested? ^^

@TheChymera
Copy link
Collaborator Author

@tsalo interested in reviewing this as well?

@tsalo
Copy link
Member

tsalo commented Jun 10, 2022

I'm struggling with this one. Could you add a unit test for load_entities's new parameters and/or _inheritance_expansion? Or even examples in the docstrings would be helpful.

@TheChymera
Copy link
Collaborator Author

@tsalo better?

@TheChymera
Copy link
Collaborator Author

@effigies interested in merging? :)

@effigies
Copy link
Collaborator

effigies commented Jun 16, 2022 •
edited
Loading

Haven't had a chance to review yet. Sorry. Might not happen until July.

@yarikoptic yarikoptic mentioned this pull request Jul 12, 2022
Merged
yarikoptic
yarikoptic requested changes Jul 12, 2022
View reviewed changes
Copy link
Collaborator

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Initial quick review

tools/schemacode/schemacode/validator.py Outdated Show resolved Hide resolved
tools/schemacode/schemacode/validator.py Outdated Show resolved Hide resolved
tools/schemacode/schemacode/validator.py Outdated Show resolved Hide resolved
tools/schemacode/schemacode/tests/test_validator.py Outdated Show resolved Hide resolved
@TheChymera TheChymera force-pushed the validator_inheritance branch 2 times, most recently from 0db0a81 to cda19eb Compare July 15, 2022 19:40
@yarikoptic
Copy link
Collaborator

for codespell I have submitted separate #1148 to fix it asap.

yarikoptic added a commit to yarikoptic/bids-specification that referenced this pull request Jul 18, 2022
@yarikoptic
[FIX] cover bidsschematools not old name schemacode
27f15c0 
renaming happened in f84cd85 but coverage submission was not adjusted.
That is what resulted (I guess) in no coverage being submitted as in
bids-standard#1120

    ==> GitHub Actions detected.
        Env vars used:
          -> GITHUB_ACTIONS:    true
          -> GITHUB_HEAD_REF:   validator_inheritance
          -> GITHUB_REF:        refs/pull/1120/merge
          -> GITHUB_REPOSITORY: bids-standard/bids-specification
          -> GITHUB_RUN_ID:     2680833203
          -> GITHUB_SHA:        b39dbd7
          -> GITHUB_WORKFLOW:   schemacode_ci
    project root: .
    Yaml not found, that's ok! Learn more athttp://docs.codecov.io/docs/codecov-yaml
    ==> Running gcov in . (disable via -X gcov)
    ==> Python coveragepy exists disable via -X coveragepy
    -> Running coverage xml
    No data to report.
    ==> Searching for coverage reports in:
    + .
    --> No coverage report found.
        Please visit http://docs.codecov.io/docs/supported-languages
This was referenced Jul 18, 2022
Merged
Open
yarikoptic
yarikoptic requested changes Jul 18, 2022
View reviewed changes
Copy link
Collaborator

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Let's possibly figure out better API for having two different modes

Also sent TheChymera#1 with some suggestions/enhancements spotted while reviewing code

tools/schemacode/bidsschematools/validator.py Show resolved Hide resolved
tools/schemacode/bidsschematools/validator.py Outdated Show resolved Hide resolved
tools/schemacode/bidsschematools/validator.py Show resolved Hide resolved
tools/schemacode/bidsschematools/validator.py Outdated Show resolved Hide resolved
This was referenced Jul 19, 2022
Closed
Closed
yarikoptic
yarikoptic approved these changes Jul 25, 2022
View reviewed changes
Copy link
Collaborator

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

I still don't like dummy_paths and question some other options naming, but others approved and we can't keep this PR hostage - will be accumulating conflicts etc. Could be RFed later (but soonish).

@TheChymera TheChymera requested a review from effigies July 26, 2022 09:23
@TheChymera
Copy link
Collaborator Author

@effigies interested in approving this as well? I knew there was something I wanted to bring up in the meeting today :)

@TheChymera
Copy link
Collaborator Author

@sappelhoff interested in merging this? :)

@sappelhoff sappelhoff added the exclude-from-changelog This item will not feature in the automatically generated changelog label Aug 2, 2022
@sappelhoff sappelhoff merged commit a0512aa into bids-standard:master Aug 2, 2022
@sappelhoff
Copy link
Member

thanks @TheChymera

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yarikoptic yarikoptic yarikoptic approved these changes

@tsalo tsalo Awaiting requested review from tsalo

@erdalkaraca erdalkaraca Awaiting requested review from erdalkaraca erdalkaraca is a code owner

@effigies effigies Awaiting requested review from effigies

Assignees
No one assigned
Labels
exclude-from-changelog This item will not feature in the automatically generated changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TheChymera @tsalo @effigies @yarikoptic @sappelhoff