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

Conventional Commit Popup #2486

Open
wants to merge 51 commits into
base: master
Choose a base branch
from
Open

Conventional Commit Popup #2486

wants to merge 51 commits into from

Conversation

tkr-sh
Copy link

@tkr-sh tkr-sh commented Jan 16, 2025

It changes the following:

  • Add a new "Conventional Commit" popup, with a feature "gitmoji" that allows quick templating messages with rich git commit messages following conventional commits.

I followed the checklist:

  • I added unittests (I don't think that there are unittests on popup usually)
  • I ran make check without errors
  • I tested the overall application
  • I added an appropriate item to the changelog

Also, I know that the UI and the UX is different than the one of Fuzzy finder, but I prefer this one personally (I think that the UX is just better tbh), and it's still easy to follow, even for non vim users :) !

Video:
asciicast
Note: the video is kinda glitched because of the emojis, but everything is aligned

@naseschwarz
Copy link
Contributor

Hi @tkr-sh. Thank you for your effort and for sharing this.

If you're still interested, would you be so kind and add a few key screenshots (or even a video capture) that describe this change so we can discuss based on this? I guess this might help this patch along. It's unlikely many people will join the discussion if they have to assert whether a 1k patch does any shenanigans on their development machines or spool up a cloud instance for every iteration and I take it that there will be discussions.

@tkr-sh
Copy link
Author

tkr-sh commented Mar 21, 2025

Sure. Tho in theory, it could still work and have untrusted code. In any case, this should be reviewed, and it's straightforward enough so that people realize that there are no shady stuff.
I'll do that in a few hours

@naseschwarz
Copy link
Contributor

Thank you!

Tho in theory, it could still work and have untrusted code.

Sure, screenshots won't change the level of trust by much. They're not so much to inspire trust but to provide the input that's needed for a discussion that can move the PR along.

@@ -76,8 +77,9 @@ chrono = { version = "0.4", default-features = false, features = ["clock"] }
maintenance = { status = "actively-developed" }

[features]
default = ["ghemoji", "regex-fancy", "trace-libgit", "vendor-openssl"]
default = ["ghemoji", "regex-fancy", "trace-libgit", "vendor-openssl", "gitmoji"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

making this default on will be controversial. I remember we did that in the past and it bloats the binary for a lot of people that are not interested in this

Copy link
Author

@tkr-sh tkr-sh Mar 23, 2025

Choose a reason for hiding this comment

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

Well, it can be true sometimes, but in this specific case I don't think that it's a good argument because

# With the feature
$ cargo build --release && du -b target/release/gitui
15038504	target/release/gitui
# Without the feature
$ cargo build --release && du -b target/release/gitui
15025560	target/release/gitui

(12944B diff ≈ 0.086% of the total binary size, tested at bb78714)

But you could make the point that most people don't want gitmoji, and in this case, removing it as a default feature is understandable.

@tkr-sh
Copy link
Author

tkr-sh commented Mar 23, 2025

Added a video and fixed/upgraded some stuff @naseschwarz @extrawurst

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