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

Do not hide error messages when trying to return early #318

Merged
merged 2 commits into from
Aug 6, 2023

Conversation

sandr01d
Copy link
Collaborator

Check list

  • I have performed a self-review of my code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation

Description

Most of forgits functions allow to invoke git directly when arguments were passed. In most of these functions we only return early when the git command that is executed returns successfully. This hides error messages from the user. As discussed with @carlfriedrich in #316, I've modified this behavior to also return early when the git command was not successful, so we're transparent about gits error messages and return values. Because gbl and gclean allow passing arguments to the git command that is invoked when we don't return early, I've modified their behavior to only invoke git directly (and return early) in the case that non flag arguments were passed.
I've also modified gclean to use the -q flag instead of piping it's output to /dev/null.
This PR supersedes #316.

Type of change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation change

Test environment

  • Shell
    • bash
    • zsh
    • fish
  • OS
    • Linux
    • Mac OS X
    • Windows
    • Others:

@sandr01d
Copy link
Collaborator Author

I've used this for a week now and did not find any issues. I'd like to merge this PR soon. Any thoughts on this?

@carlfriedrich
Copy link
Collaborator

Sorry for the late reply! Thanks a lot for your work on this @sandr01d . I haven't had time to test it out, yet, but code-wise it looks good to me.
I would suggest squashing the two commits when merging this, though, and use the really good explanation in the PR's description as a commit message.

carlfriedrich added a commit to carlfriedrich/dotfiles that referenced this pull request Jul 31, 2023
@sandr01d
Copy link
Collaborator Author

I would suggest squashing the two commits when merging this, though, and use the really good explanation in the PR's description as a commit message.

Will do.

I'll leave this hanging for a bit in case somebody wants to test this. I'll merge this next weekend when there are no objections.

carlfriedrich added a commit to carlfriedrich/dotfiles that referenced this pull request Aug 3, 2023
@sandr01d sandr01d merged commit 9b5714f into wfxr:master Aug 6, 2023
carlfriedrich added a commit to carlfriedrich/dotfiles that referenced this pull request Aug 21, 2023
@sandr01d sandr01d mentioned this pull request Apr 4, 2024
15 tasks
@sandr01d sandr01d deleted the refactor-early-out branch April 5, 2024 18:29
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.

3 participants