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

Enh/getcachednamespaces #1432

Closed
wants to merge 12 commits into from
Closed

Enh/getcachednamespaces #1432

wants to merge 12 commits into from

Conversation

oruebel
Copy link
Contributor

@oruebel oruebel commented Feb 18, 2022

Motivation

#1431

Relate to dandi/dandi-cli#917

This PR extracts the code to determine the cached extension namespace to use for validation from the main function of the command line validator to a separate function so that the code can be used outside to validate files against cached namespaces.

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This can be checked running flake8 from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

@oruebel oruebel requested a review from rly February 18, 2022 20:35
@oruebel oruebel added category: enhancement improvements of code or code behavior topic: validator issues related to validation of files labels Feb 18, 2022
@oruebel
Copy link
Contributor Author

oruebel commented Feb 23, 2022

@rly this issue would be good to address in the next minor release

@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #1432 (f5ab53d) into dev (4460a1b) will decrease coverage by 0.08%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##              dev    #1432      +/-   ##
==========================================
- Coverage   78.42%   78.34%   -0.09%     
==========================================
  Files          37       37              
  Lines        2777     2780       +3     
  Branches      493      493              
==========================================
  Hits         2178     2178              
- Misses        518      521       +3     
  Partials       81       81              
Impacted Files Coverage Δ
src/pynwb/validate.py 0.00% <0.00%> (ø)

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 4460a1b...f5ab53d. Read the comment docs.

@rly
Copy link
Contributor

rly commented Mar 17, 2022

@oruebel what do you think about also extracting most of the code in __main__ in src/pynwb/validate.py to a function that takes as inputs the arguments passed into the CLI function. Then a user can basically call the CLI script using Python without using the shell.

@oruebel
Copy link
Contributor Author

oruebel commented Mar 17, 2022

what do you think about also extracting most of the code in __main__ in src/pynwb/validate.py to a function

that makes sense

@bendichter
Copy link
Contributor

We agreed to incorporate this feature into validate

@yarikoptic
Copy link
Contributor

would this PR be merged/released some time soon?

@CodyCBakerPhD
Copy link
Collaborator

@yarikoptic I'm close to finishing something very similar that also integrates with the dandi.pynwb_utils.validate, look for that in next day or so

Copy link

@jwodder jwodder left a comment

Choose a reason for hiding this comment

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

Pointed out some minor coding improvements.

src/pynwb/validate.py Outdated Show resolved Hide resolved
src/pynwb/validate.py Outdated Show resolved Hide resolved
rly and others added 2 commits June 30, 2022 12:05
Co-authored-by: John T. Wodder II <[email protected]>
Co-authored-by: John T. Wodder II <[email protected]>
@rly
Copy link
Contributor

rly commented Jun 30, 2022

Pointed out some minor coding improvements.

Thanks, @jwodder !

@rly rly mentioned this pull request Jul 8, 2022
6 tasks
@yarikoptic
Copy link
Contributor

this one is open since July -- any plans to finalize/release it ?

@oruebel
Copy link
Contributor Author

oruebel commented Nov 1, 2022

this one is open since July -- any plans to finalize/release it ?

@yarikoptic thanks for following up. I think this one got put on hold while #1494 was being worked out and I think that PR may potentially already address that issue. I'll bring it up during our developer meeting tomorrow to see whether this PR should be merged or whether it should be closed in favor of #1494

@CodyCBakerPhD
Copy link
Collaborator

@yarikoptic This has indeed already been merged in #1511, this can be closed here and I'll get a PR up on the dandi-cli to adjust the API calls to have this functionality

@oruebel
Copy link
Contributor Author

oruebel commented Nov 1, 2022

Closing this PR since this has been addressed in #1511

@oruebel oruebel closed this Nov 1, 2022
@oruebel oruebel deleted the enh/getcachednamespaces branch November 1, 2022 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: enhancement improvements of code or code behavior topic: validator issues related to validation of files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants