search

wfxr / forgit

:zzz: A utility tool powered by fzf for using git interactively.
MIT License
4.32k stars 136 forks source link

Refactor: Replace deferred code used for fzf preview with functions #349

Closed sandr01d closed 3 months ago

sandr01d commented 4 months ago

Check list

Description

Removes the deferred code that is used for creating the fzf preview functions and replaces it with _forgit_*_preview functions instead. This PR 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. The only implementation detail I changed in comparison to #326 is that I removed xargs from from _forgit_stash_show_preview, as discussed in #343. xargs was actually not necessary here because we always preview only a single stash. This way we do not have to expose _forgit_git_stash_show as a private_command. I've included new functions we've added in #326, that are used inside the _forgit_*_preview functions. I regard them as part of the preview functions.

Type of change

Test environment

Note

This PR belongs to https://github.com/wfxr/forgit/pull/326 and resulted from discussions in https://github.com/wfxr/forgit/issues/324

sandr01d commented 4 months ago

I did all the changes I meant to do here. Awaiting your reviews @carlfriedrich @cjappl.

cjappl commented 4 months ago

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

carlfriedrich commented 4 months ago

@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. 😁

cjappl commented 4 months ago

squash all the changes into one commit

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?

carlfriedrich commented 4 months ago

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.

@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.

cjappl commented 4 months ago

Sweet, thanks for the explanation @carlfriedrich !

@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.

Nice, good point, I hadn't considered that. "Make commit messages reviewable"

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.

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.

carlfriedrich commented 4 months ago

Ah wait, we have #343 next in the line. @sandr01d Will you rebase that on top of this PR then first?

sandr01d commented 4 months ago

Yes, sure. Don't have the time today, but plan to do so tomorrow.

sandr01d commented 3 months ago

Changes have been merged with #326