Skip to content

Conversation

@candleindark
Copy link
Member

@candleindark candleindark commented Nov 16, 2025

This addresses checkpoint 2 of #1597. It adds two options to dandi validate specified in the "Release Notes" section below.

TODO:

  • Decide whether to add or minor or patch label.

Release Notes

The dandi validate now has two additional options

  1. --match which takes in a list of comma-separated regex patterns to filter issues in validation results by their ID.
  2. --include-path which takes in multiple paths to filter issues in the validation results to those associated with the paths.
@codecov
Copy link

codecov bot commented Nov 16, 2025

Codecov Report

❌ Patch coverage is 96.79144% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.38%. Comparing base (8492260) to head (9c67a46).
⚠️ Report is 17 commits behind head on master.

Files with missing lines Patch % Lines
dandi/cli/base.py 69.23% 4 Missing ⚠️
dandi/utils.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@ ## master #1743 +/- ## ========================================== + Coverage 75.08% 75.38% +0.30%  ========================================== Files 84 85 +1 Lines 11874 12041 +167 ========================================== + Hits 8915 9077 +162  - Misses 2959 2964 +5 
Flag Coverage Δ
unittests 75.38% <96.79%> (+0.30%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
@candleindark
Copy link
Member Author

@yarikoptic Is it appropriate to add the minior label for this PR? I am not sure because the minor of version is functioning more or less like the major as for other packages.

@candleindark candleindark force-pushed the enh-validate branch 4 times, most recently from 6f5d9e8 to 51a7e1c Compare November 21, 2025 05:57
@candleindark candleindark requested a review from Copilot November 21, 2025 06:06
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds two new filtering options to the dandi validate command to provide more granular control over validation results:

  • --match: Filters validation issues by ID using comma-separated regex patterns
  • --include-path: Filters validation issues by their associated file paths

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
docs/source/cmdline/validate.rst Documents the two new command-line options (--match and --include-path)
dandi/utils.py Implements filter_by_id_patterns() and filter_by_paths() helper functions with TYPE_CHECKING import for ValidationResult
dandi/cli/base.py Adds _compile_regex() and parse_regexes() callback for parsing comma-separated regex patterns
dandi/cli/cmd_validate.py Integrates new options into the validate command and applies filters in _process_issues()
dandi/tests/test_utils.py Comprehensive test coverage for both filter functions with various edge cases
dandi/cli/tests/test_cmd_validate.py Tests for CLI option parsing, validation, and interaction with existing options
dandi/cli/tests/test_base.py Tests for regex compilation and parsing utilities

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

candleindark and others added 3 commits November 20, 2025 22:27
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@candleindark candleindark marked this pull request as ready for review November 21, 2025 16:51
callback=parse_regexes,
)
@click.option(
"--include-path",
Copy link
Member

Choose a reason for hiding this comment

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

  • name was a little confusing as I thought "include name in the result"
  • we might want to simply (ab)use existing paths posarg as it is to define what paths we care about. So even if within bids dataset, bids-validator-deno would run on entire dataset ATM and then we need to filter for those paths
    • we should also warn about that -- that we do run bids-validator on the full dandiset although interested only in some
)
@click.option("--ignore", metavar="REGEX", help="Regex matching error IDs to ignore")
@click.option(
"--match",
Copy link
Member

Choose a reason for hiding this comment

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

--match suggests me to match as limiting some work... here we are talking only report filtering... I also wonder if we could join here to support various elements of an issue, not have them separate, so could be smth like

--report-matches=id:REGEX,path:.\*_events.* 

which should serve as AND so should match all of the above

),
]

@pytest.mark.parametrize(
Copy link
Member

Choose a reason for hiding this comment

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

these all look like AI generated... please instruct your agent to use @pytest.mark.ai_generated for such tests

Copy link
Member

@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 left comments in line, without compiling into this report. I will move into draft to avoid paying attention until the next round (take out of draft then)

@yarikoptic yarikoptic added the minor Increment the minor version when merged label Dec 1, 2025
@yarikoptic yarikoptic marked this pull request as draft December 1, 2025 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd-validate minor Increment the minor version when merged

3 participants