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

Update README.md to make long vs short flags clearer #253

Merged
merged 3 commits into from
Jan 17, 2024

Conversation

DanielTsiang
Copy link
Contributor

@DanielTsiang DanielTsiang commented Jan 17, 2024

Update README.md to make use of long vs short flags clearer, and fix copy-paste typo in the bottom table for ignoring notebook metadata.

I mistakenly just copy and pasted the command from the table but then was confused why it wasn't working...better to make it more foolproof 🙂

This PR addresses the following issue:

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (80640b4) 99.46% compared to head (740a78f) 99.46%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #253   +/-   ##
=======================================
  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.

@DanielTsiang
Copy link
Contributor Author

DanielTsiang commented Jan 17, 2024

This pull request may be merged without approvals.

Only those with write access to this repository can merge pull requests.

@srstevenson thanks for looking at my PR, could you help merge it please?

Copy link
Owner

@srstevenson srstevenson left a comment

Choose a reason for hiding this comment

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

Hi @DanielTsiang, thanks for contributing! I agree, that replacing the slashes with or makes it clearer to readers how they should specify these options.

I've reverted the change to the migration table though: this flag should remain --preserve-cell-metadata as the support for preserving notebook metadata and the corresponding --preserve-notebook-metadata flag was only added after the v2.0.0 release, and hence don't appear in the migration table. The reason there are two rows in the migration table mentioning --preserve-cell-metadata is because one is demonstrating it for the check subcommand, and one for the clean subcommand.

This flag should remain `--preserve-cell-metadata` as the support
for preserving notebook metadata and the corresponding
`--preserve-notebook-metadata` flag was only added after the v2.0.0
release, and hence don't appear in the migration table.
@srstevenson srstevenson merged commit a7b8f57 into srstevenson:main Jan 17, 2024
@srstevenson srstevenson changed the title Update README.md to make long vs short flags clearer, and fix typo Update README.md to make long vs short flags clearer Jan 17, 2024
@DanielTsiang
Copy link
Contributor Author

DanielTsiang commented Jan 18, 2024

@srstevenson That makes sense, thanks for explaining!

From your reply on the issue here, I only just realised that the flag can go before or after the notebook filename (but has to go after clean or check). It wasn't obvious to me from the docs.

I personally think this:

$ nb-clean check --preserve-notebook-metadata my-notebook.ipynb

is a lot neater/simpler than this (which is how it's written in the docs), especially when using it in the CI and you're passing filenames to the check or clean command via xargs:

$ nb-clean check my-notebook.ipynb --preserve-notebook-metadata

@DanielTsiang DanielTsiang deleted the patch-1 branch January 18, 2024 02:07
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