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

Document command line flag corner cases more clearly #254

Merged
merged 3 commits into from
Jan 19, 2024

Conversation

DanielTsiang
Copy link
Contributor

@DanielTsiang DanielTsiang commented Jan 18, 2024

Make README.md clearer.

Changes:

  • Add more examples of how to use flags with clean and check modes.
  • Rewrite flags section into bullet points, easier to digest information flag by flag instead of all in one long paragraph.
  • Reorder the flags at the table at the end to put the flag before the filename, which is neater. It's also simpler to use if you're using nb-clean in the CI and you're piping notebook filenames to the command via xargs.

@DanielTsiang DanielTsiang deleted the patch-2 branch January 18, 2024 02:06
@DanielTsiang DanielTsiang restored the patch-2 branch January 18, 2024 02:07
@DanielTsiang DanielTsiang reopened this Jan 18, 2024
@DanielTsiang
Copy link
Contributor Author

Might want to hold off merging this PR in actually until this bug is resolved:

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a7b8f57) 99.46% compared to head (4b71789) 99.46%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #254   +/-   ##
=======================================
  Coverage   99.46%   99.46%           
=======================================
  Files           3        3           
  Lines         187      187           
=======================================
  Hits          186      186           
  Misses          1        1           

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

@srstevenson
Copy link
Owner

Thanks, @DanielTsiang, this makes things clearer! I've added a commit to give more detailed instructions about avoiding the issue you experienced in #255. Note I reverted your change to the nb-clean 1.6.0 column in the migration table: in 1.6.0, the notebook path was passed with the --input flag and not as a positional argument, so can't be moved to the end of the command line.

- Add more examples of how to use flags with clean and check modes.
- Rewrite flags section into bullet points, easier to digest
  information flag by flag instead of all in one long paragraph.
- Reorder the flags at the table at the end to put the flag before
  the filename, which is neater and makes more sense.
@srstevenson srstevenson force-pushed the patch-2 branch 2 times, most recently from ff4eea3 to 2f95ed5 Compare January 19, 2024 23:26
Because `--preserve-cell-metadata` can take zero or more arguments,
the argument parser assigns any positional arguments following this
flag as fields to preserve rather than notebook paths to process.
This means if you're not passing any custom fields to preserve, the
`--preserve-cell-metadata` flag needs to appear after the paths to
be processed, or the notebook path should be preceded with `--` to
tell the argument parser it's not an argument to
`--preserve-cell-metadata`.
In nb-clean 1.6.0, the notebook path was not a position argument but had
to be passed to the `--input` flag, so the syntax given would not work.
@srstevenson srstevenson changed the title Make README.md clearer Document command line flag corner cases more clearly Jan 19, 2024
@srstevenson srstevenson merged commit b71adf0 into srstevenson:main Jan 19, 2024
7 checks passed
@DanielTsiang
Copy link
Contributor Author

Thanks for correcting the PR!

The README.md is a lot clearer now so hopefully a lot more people will feel comfortable using this tool in the future! 🙂

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.

2 participants