Skip to content

Name the --overwrite flag in file-exists errors - #749

Merged
bendichter merged 6 commits into
devfrom
fix/overwrite-flag-message
Sep 16, 2026
Merged

bendichter merged 6 commits into
devfrom
fix/overwrite-flag-message

Conversation

@bendichter

Copy link
Copy Markdown
Contributor

Fixes #748.

The "already exists" errors raised by save_report and by the CLI for --json-file-path told the user to pass -o, which is not a flag the CLI defines. Both now say --overwrite.

One test calls save_report against an existing file and matches on --overwrite; the other drives the CLI with click's CliRunner against an existing JSON path and checks the exception text. Both fail on dev.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Hfn8dhw9cwP5FiVaCgFmCt

Both messages told the user to pass '-o', which the CLI does not define.

Closes #748

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hfn8dhw9cwP5FiVaCgFmCt

@h-mayorquin h-mayorquin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The fix is good, thanks for catching this (the message was sending people to a flag that errors out!).

One thing we could improve here. Both existence checks run after the whole inspection is done. The json_file_path one is after messages = list(messages_iterator) and the save_report one is later still, and neither of them needs the messages, they are only looking at a path. If we move them up to the top with the rest of the argument handling then a user who points --json-file-path at a file that already exists finds out right away instead of after scanning a whole dandiset.

That would also make test_cli_existing_json_file_message_names_overwrite_flag a lot lighter. Right now it writes an NWB file with NWBHDF5IO and runs the whole CLI to check a substring, and with the check at the top it does not need a file at all.

Benjamin Dichter and others added 4 commits September 16, 2026 11:09
Mechanical conflict resolution: CHANGELOG.md [changelog]
The changelog is now assembled from one file per entry (#764), so the
entry moves out of CHANGELOG.md into a file named after this pull
request.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Both existence checks ran after the whole inspection, so a user who
pointed --json-file-path or --report-file-path at an existing file
found out only after scanning a whole folder or dandiset. They now run
with the rest of the argument handling, and the CLI tests no longer
need an NWB file to reach them.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@bendichter

Copy link
Copy Markdown
Contributor Author

Good call. Both checks now run with the rest of the argument handling, before anything is inspected, and they cover --report-file-path as well as --json-file-path. The late check on the JSON path is gone since it could no longer fire; the one inside save_report stays for callers of the library. The CLI tests now point the command at an empty folder, so no NWB file is written, and there is a third test that runs with --overwrite to confirm both files are then rewritten.

@bendichter
bendichter enabled auto-merge (squash) September 16, 2026 19:48

@h-mayorquin h-mayorquin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, thanks for addressing the feedback.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.90%. Comparing base (e5001c1) to head (1d386e8).
⚠️ Report is 4 commits behind head on dev.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #749      +/-   ##
==========================================
+ Coverage   81.26%   88.90%   +7.64%     
==========================================
  Files          48       48              
  Lines        1916     1938      +22     
==========================================
+ Hits         1557     1723     +166     
+ Misses        359      215     -144     
Files with missing lines Coverage Δ
src/nwbinspector/_formatting.py 83.44% <100.00%> (+13.10%) ⬆️
src/nwbinspector/_nwbinspector_cli.py 78.16% <100.00%> (+78.16%) ⬆️

... and 4 files with indirect coverage changes

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

@bendichter
bendichter merged commit 2e5eefd into dev Sep 16, 2026
25 checks passed
@bendichter
bendichter deleted the fix/overwrite-flag-message branch September 16, 2026 21:59
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.

Overwrite error messages refer to a -o flag that does not exist

3 participants