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

Allow creation of annotated tags #1469

Merged
merged 10 commits into from
Dec 26, 2021

Conversation

fsmiamoto
Copy link
Contributor

@fsmiamoto fsmiamoto commented Sep 11, 2021

What

Allow the creation of annotated tags.
This implementation only supports adding a message to the tag but it could be extended in the future.

Why

This was requested in #1381 as currently we only support creating the so called lightweight tags

@fsmiamoto
Copy link
Contributor Author

fsmiamoto commented Sep 11, 2021

@jesseduffield This PR still has some rough edges but let me know what you think of the general approach.

Looks like most of tests related to tags broke since we would now have a menu before the creation so if we're going in this direction, I'll make sure to fix them :)

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

nice work! Couple small things

pkg/gui/commits_panel.go Outdated Show resolved Hide resolved
pkg/i18n/english.go Outdated Show resolved Hide resolved
pkg/i18n/english.go Outdated Show resolved Hide resolved
@fsmiamoto fsmiamoto force-pushed the feature/annotated_tags branch 2 times, most recently from ce77fcb to 208ed7b Compare September 12, 2021 20:28
@fsmiamoto
Copy link
Contributor Author

Took the opportunity to give some updates to the docs on Integration Tests as the commands there were not working as expected.

Not sure if what I did to fix the tests is the right thing but here's what I've done:

  • Run the Integration Test TUI with go run ./test/lazyintegration/main.go
  • Re-record the tests related to tags, trying/guessing what was done at those tests - not sure if there's a good way to visualize the actions that were there before.

Although these tests are slow by their own nature, I've really liked the general approach!
Some things are not quite there in the Integration Test TUI - the test list didn't scroll properly for me - so I might give some help there afterwards.

@fsmiamoto
Copy link
Contributor Author

fsmiamoto commented Sep 12, 2021

Also, not sure why the linting is failing on me 😅
Maybe a different gofmtversion on the CI? I'm not able to reproduce it locally with go 1.16.

@fsmiamoto fsmiamoto marked this pull request as ready for review September 12, 2021 20:36
@fsmiamoto
Copy link
Contributor Author

Linting problems were caused by Go 1.17, fixed at #1479.

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

Nice work, left a couple comments

func (gui *Gui) afterTagCreate(tagName string) error {
return gui.refreshSidePanels(refreshOptions{mode: ASYNC, scope: []RefreshableView{COMMITS, TAGS}, then: func() {
// find the index of the tag and set that as the currently selected line
for i, tag := range gui.State.Tags {
Copy link
Owner

Choose a reason for hiding this comment

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

now that we sort tags by date created, descending, we can just set the cursor to the top. We don't even need to do that in a then callback: we can do it immediately

Copy link
Contributor Author

@fsmiamoto fsmiamoto Oct 28, 2021

Choose a reason for hiding this comment

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

Sure!

return gui.prompt(promptOpts{
title: gui.Tr.TagNameTitle,
handleConfirm: func(tagName string) error {
return gui.prompt(promptOpts{
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder here whether a single-line editor is going to suffice. I assume that people typically use annotated tags for the sake of writing release notes for a release, and that will require more than one line. At any rate I'm happy to release this and see if anybody wants multi-line just so that this code doesn't get stale

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I agree, we can definitely extend this later on.

@fsmiamoto
Copy link
Contributor Author

The pushTag related tests seem to be failing, I'll take a look at those.

@fsmiamoto
Copy link
Contributor Author

Fixed the pushTag by adding an extra Enter input. Should be good to go now :)

@jesseduffield
Copy link
Owner

Just tested and it works fine. Nice work!

@jesseduffield jesseduffield merged commit 2696a63 into jesseduffield:master Dec 26, 2021
@fsmiamoto fsmiamoto deleted the feature/annotated_tags branch December 27, 2021 02:35
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.

None yet

2 participants