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

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

Closed
alonme opened this issue Jun 8, 2021 · 12 comments
Closed

Comments

@alonme
Copy link

alonme commented Jun 8, 2021

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
Copy link
Owner

kynan commented Jun 8, 2021

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

@alonme
Copy link
Author

alonme commented Jun 9, 2021

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
Copy link
Owner

kynan commented Jun 9, 2021

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
Copy link

ghost commented Jan 20, 2022

It would be awesome to have this option.

@kynan
Copy link
Owner

kynan commented Jan 23, 2022

Feel like contributing this feature @yliederNY ? :)

@kynan
Copy link
Owner

kynan commented Sep 24, 2022

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

@jspaezp
Copy link
Contributor

jspaezp commented Nov 5, 2023

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
Copy link
Owner

kynan commented Nov 12, 2023

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

@kmcentush
Copy link

Also would love this functionality.

@jspaezp
Copy link
Contributor

jspaezp commented Feb 14, 2024

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. -> #192 reference (it is off a fairly old branch so it cannot be merged RN but the idea is the same ..)

@kynan
Copy link
Owner

kynan commented Feb 17, 2024

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 kynan added the state:pr pending This issue has a pending pull request label Feb 17, 2024
This was referenced Feb 18, 2024
@jspaezp
Copy link
Contributor

jspaezp commented Feb 18, 2024

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

@kynan kynan modified the milestones: Backlog, 0.8.0 Nov 3, 2024
@kynan kynan added resolution:fixed and removed help wanted state:pr pending This issue has a pending pull request labels Nov 3, 2024
@kynan kynan closed this as completed in b8fd98c Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants