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

Refactor: Replace _forgit_emojify deffered code variable with a function #352

Closed
wants to merge 4 commits into from

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

We used to have a variable that was either undefined or contained a piece of deferred code that piped input through emojify when present on the system. To remove the deferred code here, this commit replaces the _forgit_emojify variable with a function that either pipes the input through emojify or through cat, depending on whether emojify is present.

Note

This PR belongs to #326 and resulted from discussions in #324.
In comparison to #326 I've removed passing the function arguments to _forgit_emojify using "$@" because that was unnecessary. Both emojify and cat read input from stdin and the _forgit_emojify function does not take any arguments.

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:

Forgit allows specifying git options in environment variables that are passed along to the individual git commands. We currently treat those as strings. This commit adds a _forgit_parse_array function and uses it to parse all such environment variables into arrays instead. This will allow us to get rid of deferred code, since we can pass the parsed arrays directly to the git commands and don't have to rely on eval.
Removes the deferred code that is used for creating the fzf preview functions and replaces it with _forgit_*_preview functions instead. These functions are exposed as forgit commands so they can be invoked from the fzf subshell. We split the exposed commands into public_commands and private_commands. The only difference between them is that public_commands are mentioned in the help text.
This commit changes the flags variable in _forgit_blame from a string to an array. This is necessary to allow the flags to be passed to _forgit_blame_preview as individual arguments.
We often used deferred code to encapsulate git commands and make them
reusable.
This change removes deferred code for git commands and replaces it with
functions instead.
Some of the deferred code was used with xargs, which executes it on a
subshell. To avoid having to expose the new git functions the same way
we do with the preview functions, the usage of xargs in these cases is
replaced with either a loop or a single command when possible.
@sandr01d
Copy link
Collaborator Author

We're making good progress here. Rebasing now actually removes commits, meaning that some functions do not have any deferred code left 🙂
I expect the following PRs to be smaller than the previous ones. This one should be fairly easy. Looking forward to your reviews @cjappl @carlfriedrich @wfxr

Copy link
Collaborator

@carlfriedrich carlfriedrich left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot!

@carlfriedrich
Copy link
Collaborator

Oh, I just noticed a typo in the commit msg: should be "deferred" instead of "deffered".

We used to have a variable that was either undefined or contained a
piece of deferred code that piped input through emojify when present
on the system. To remove the deferred code here, this commit
replaces the _forgit_emojify variable with a function that either pipes
the input through emojify or through cat, depending on whether emojify
is present.
@sandr01d sandr01d changed the title Replace _forgit_emojify deffered code variable with a function Refactor: Replace _forgit_emojify deffered code variable with a function Feb 27, 2024
Copy link
Owner

@wfxr wfxr left a comment

Choose a reason for hiding this comment

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

LGTM.

@sandr01d
Copy link
Collaborator Author

Changes have been merged with #326

@sandr01d sandr01d closed this Mar 24, 2024
@sandr01d sandr01d deleted the emojify branch April 5, 2024 18:26
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