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

Pedantic warning: Return != 0 when a hook fails #112

Merged
merged 18 commits into from
Dec 1, 2021
Merged

Conversation

pschmitt
Copy link
Member

Fixes #111

This also logs an explicit warning when a hook fails:

Warning: ∞zinit-compile-plugin-hook hook returned with 1

@pschmitt pschmitt requested a review from alichtman November 26, 2021 22:57
@pschmitt
Copy link
Member Author

pschmitt commented Nov 26, 2021

Surprise, surprise: this makes the zunit tests fail ;)

There's still a bit more work to do here.

DO NOT MERGE

@pschmitt pschmitt marked this pull request as draft November 26, 2021 23:36
@pschmitt
Copy link
Member Author

Okay, this was easier than I thought.

This is still a potentially breaking change, since it makes zinit quite a bit more strict in regards to return codes.

@pschmitt pschmitt added the enhancement New feature or request label Nov 26, 2021
@pschmitt pschmitt added this to the zinit 4.0 milestone Nov 26, 2021
@alichtman
Copy link
Member

This is still a potentially breaking change, since it makes zinit quite a bit more strict in regards to return codes.

I'm cool with a major version bump if this could change behavior of other programs. We've done some pretty heavy modification, so it's getting time for a major release anyways.

@pschmitt
Copy link
Member Author

pschmitt commented Nov 27, 2021

so it's getting time for a major release anyways.

Hence the v4.0 milestone :)

Copy link
Member

@alichtman alichtman left a comment

Choose a reason for hiding this comment

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

image

zinit-install.zsh Show resolved Hide resolved
zinit-install.zsh Show resolved Hide resolved
zinit-install.zsh Show resolved Hide resolved
@pschmitt pschmitt mentioned this pull request Nov 29, 2021
@pschmitt
Copy link
Member Author

pschmitt commented Nov 29, 2021

This needs a dedicated ci check.

zinit null atclone'return 69' for zdharma-continuum/null

's RC should match 69 in zunit

@pschmitt
Copy link
Member Author

pschmitt commented Nov 29, 2021

Done (see the ices zunit test)

Also: I've forwarded the rc even further. You get the actual != 0 rc of the failing ice now instead of a static 1 when anything goes wrong.

@pschmitt pschmitt changed the title Return != 0 when a hook fails Pedantic warning: Return != 0 when a hook fails Nov 29, 2021
@pschmitt pschmitt marked this pull request as ready for review November 29, 2021 11:03
@pschmitt
Copy link
Member Author

Update: zinit update return the right rcs now

@pschmitt
Copy link
Member Author

Forgot to mention: the atpull ice now has a dedicated zunit test and correctly forwards its rc to zinit

@pschmitt
Copy link
Member Author

Okay, docs updated and all. This is ready IMO.
Any last words?

@pschmitt pschmitt merged commit 1e6bdf0 into main Dec 1, 2021
@pschmitt pschmitt deleted the return-hook-rc branch December 1, 2021 07:49
@fhilgers
Copy link

fhilgers commented Dec 1, 2021

In zinit-install.zsh the Function zinit-mv-hook can never return 0 as one of

command mv -vf "${afr[1]}" "$to"
command mv -vf "${afr[1]}".zwc "$to".zwc 2>/dev/null

will always return 1.
So zinit always shows that an error happened on the mv-hook even though it ran successfully.

@pschmitt
Copy link
Member Author

pschmitt commented Dec 1, 2021

Thanks for the hint. Will fix that.

Edit: #126

@github-actions
Copy link

github-actions bot commented Nov 7, 2022

🎉 This PR is included in version 3.8.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

💥 atclone's exit code shoud be propagated to the parent zinit call
3 participants