search

kynan / nbstripout

strip output from Jupyter and IPython notebooks
Other
1.19k stars 94 forks source link

Add an option to return an error code if any files have/should be changed #153

Open alonme opened 3 years ago

alonme commented 3 years ago

In order to use this tool in CI, it would be useful if it could return an error code when a file has been changed/ would have been changed (when using dry_run)

kynan commented 3 years ago

For use in CI you probably want to use nbstripout as a pre-commit hook.

alonme commented 3 years ago

I am not using (and currently cannot use) pre-commit ci.

Also - i don't want to alter files in my CI (server side, not locally before commits).

So i need to get some indication regarding if files were changed, which is AFAIU not currently available from the tool?

kynan commented 3 years ago

No, this is not currently support. It's not necessary when used as a pre-commit hook, as the default behavior of the hook is to fail if any changes would have been made.

What you're asking for I'd probably think of as a --verify option. The difficulty in implementing this is the same as mentioned in #152 .

Adding this as a feature request to the backlog.

ghost commented 2 years ago

It would be awesome to have this option.

kynan commented 2 years ago

Feel like contributing this feature @yliederNY ? :)

kynan commented 1 year ago

@alonme @yliederNY one of you interested in working on this?

jspaezp commented 9 months ago

Hello there!

I like the idea of a --verify flag! I will try to make a PR for it sometime soon! Just so we are clear, the. --verify would act as dry run but exit with status 1 when at least one of the files would have been stripped, and should be consistent to --dry-run in terms of stdout/stderr, right?

best!

kynan commented 9 months ago

@jspaezp Yes, what you describe sounds reasonable to me 👍🏽

kmcentush commented 6 months ago

Also would love this functionality.

jspaezp commented 6 months ago

I looked into it but with the current structure it leads to a lot of branching. It is doable but without some refactoring the ending code is very ugly. -> https://github.com/kynan/nbstripout/pull/192 reference (it is off a fairly old branch so it cannot be merged RN but the idea is the same ..)

kynan commented 6 months ago

Thanks @jspaezp for looking into this! As mentioned on your PR it's unfortunately not currently in a reviewable state. Could you please rebase on the tip of main?

jspaezp commented 6 months ago

Just sent a PR with the new main (after the refactor done it was a lot easier to add the feature, thanks for the code cleanup!)