Refactor: Replace deferred code used for fzf preview with functions #349
Conversation
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.
|
I did all the changes I meant to do here. Awaiting your reviews @carlfriedrich @cjappl. |
|
Sounds good! A lot to review, but I'll take it through it's paces on fish as a system test today and report back on how that goes :) |
|
@sandr01d Thanks for the patch! I will do a review in the following days. This time I'll make sure to view it directly on my desktop monitor. 😁 |
There was a problem hiding this comment.
Great work @sandr01d, thanks for extracting this patch! Can you fix the few things I requested and then squash all the changes into one commit? Thanks a lot!
There was a problem hiding this comment.
From a "system test" perspective, fish behaves as expected with all of the major commands on MacOS, so I'm approving! Good work. I support @carlfriedrich's suggestions :)
Is there an advantage to doing this before "squash and merge" when we merge the final one @carlfriedrich ? You keep suggesting it, which makes me think I'm overlooking some upside to the approach. In my perspective, it seems like manually squashing and merging is unneeded extra work if the work will just be squashed-and-merged automagically by github later. Is there a difference in the two approaches that makes the manual way better? |
@cjappl Generally speaking, it gives us the possibility to review the commit message. This is not the case when GitHub's squash and merge feature is used, because that happens after the review. And since the commit message is used for the changelog in our release notes, it should be part of the review IMO, especially given the fact that it cannot be changed once it is merged to master. In this specific case it is also crucial that we don't want to squash all the commits, since this PR also contains the base commit which will be merged in another PR. This makes it even more important to keep the commits tidy. |
|
Sweet, thanks for the explanation @carlfriedrich !
Nice, good point, I hadn't considered that. "Make commit messages reviewable"
Makes sense to me! I think in this specific review case (considering the complexity of it all) I'm all for anything that provides reviewability for it. I'm pro-squash for this review. I don't necessarily think we need to do this for 100% of PRs in forgit, but we can maybe discuss on an Issue ticket if we want to implement that process change for every commit going forward, I think there are mostly "pros" for complex multi-file multi-commit branches, and mostly "cons" for simple changes. |
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.
|
Yes, sure. Don't have the time today, but plan to do so tomorrow. |
|
Changes have been merged with #326 |
Check list
Description
Removes the deferred code that is used for creating the fzf preview functions and replaces it with
_forgit_*_previewfunctions instead.This PR changes the
flagsvariable in_forgit_blamefrom a string to an array. This is necessary to allow the flags to be passed to_forgit_blame_previewas individual arguments.The only implementation detail I changed in comparison to #326 is that I removed
xargsfrom from_forgit_stash_show_preview, as discussed in #343.xargswas actually not necessary here because we always preview only a single stash. This way we do not have to expose_forgit_git_stash_showas aprivate_command.I've included new functions we've added in #326, that are used inside the
_forgit_*_previewfunctions. I regard them as part of the preview functions.Type of change
Test environment
Note
This PR belongs to #326 and resulted from discussions in #324