nbstripout icon indicating copy to clipboard operation
nbstripout copied to clipboard

Published 20 hours ago • verified publisher icon kynan

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

Open alonme opened this issue 3 years ago • 12 comments

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)

alonme avatar Jun 08 '21 08:06 alonme

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

kynan avatar Jun 08 '21 22:06 kynan

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?

alonme avatar Jun 09 '21 05:06 alonme

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.

kynan avatar Jun 09 '21 18:06 kynan

It would be awesome to have this option.

ghost avatar Jan 20 '22 16:01 ghost

Feel like contributing this feature @yliederNY ? :)

kynan avatar Jan 23 '22 21:01 kynan

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

kynan avatar Sep 24 '22 12:09 kynan

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!

jspaezp avatar Nov 05 '23 17:11 jspaezp

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

kynan avatar Nov 12 '23 15:11 kynan

Also would love this functionality.

kmcentush avatar Feb 13 '24 23:02 kmcentush

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 ..)

jspaezp avatar Feb 14 '24 00:02 jspaezp

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?

kynan avatar Feb 17 '24 12:02 kynan

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!)

jspaezp avatar Feb 18 '24 20:02 jspaezp