Add runCommand function to Go template syntax + add support for templates in git branchPrefix setting

[ruudk]

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

This makes it possible to use date and time in initial values like this:

initialValue: 'ruudk/{{ runCommand "date +\"%Y/%-m\"" }}/'

I want to use this to configure my BranchPrefix like this:

git:
  branchPrefix: 'ruudk/{{ runCommand "date +\"%Y/%-m\"" }}/'

[ruudk]

@jesseduffield I know you're probably busy, but do you have any advice on where to add the tests? Then I can finish the PR.

Thanks for this amazing program, I love it!

[stefanhaller]

Happy to step in for Jesse, but I don't understand what you're trying to do. 😅

I understand the example with the initialValue: in custom command prompts; that's fine with me. Just one detail: the function takes an argument of time.Time, but the .Now property seems to be the only way to get one. How can the function ever be called differently than formatDate .Now? I guess my point is whether we should instead have something like currentFormattedDate "2006" or something along those lines, so that we don't pretend to offer more flexibility than we actually do.

What I don't understand is the branchPrefix example. We don't expand templates in that context, or do we? Are you planning on adding that; if so, how? Your exported formatDate function is very specific to custom commands.

[ruudk]

I understand the example with the initialValue: in custom command prompts; that's fine with me. Just one detail: the function takes an argument of time.Time, but the .Now property seems to be the only way to get one. How can the function ever be called differently than formatDate .Now? I guess my point is whether we should instead have something like currentFormattedDate "2006" or something along those lines, so that we don't pretend to offer more flexibility than we actually do.

That's a good suggestion. What about formatCurrentDate or formatCurrentDateTime?

What I don't understand is the branchPrefix example. We don't expand templates in that context, or do we? Are you planning on adding that; if so, how? Your exported formatDate function is very specific to custom commands.

You're right. My idea was to work on adding support for that next, in a similar way.

Before this PR, I had this custom command to create a branch (using git town)

      - context: "files, localBranches, commits, subCommits"
        key: "a"
        description: "Git Town Hack"
        prompts:
          - type: "input"
            title: "Branch name (ruudk/yyyy/m will be added automatically)."
            key: "BranchName"
        command: 'git town hack "ruudk/$(date +"%Y/%-m")/{{.Form.BranchName}}"'
        stream: true

As you can see, the title now has to say something like ruudk/yyyy/m while it would have been nicer to see the actual year and month.

Even better, to use it in initialValue so that you can modify it, for situations where you don't want the prefix.

With the current PR, you can use the formatted dates in the initialValue and title of a custom command.

[ruudk]

Based on:

      - context: "files, localBranches, commits, subCommits"
        key: "a"
        description: "Git Town Append"
        prompts:
          - type: "input"
            title: 'Branch name to append from "{{.CheckedOutBranch.Name}}"'
            key: "BranchName"
            initialValue: 'ruudk/{{ formatCurrentDate "2006/1" }}/'
        command: 'git town append {{.Form.BranchName | quote}}'
        stream: true
        loadingText: "Appending"

[ruudk]

Also made the branchPrefix work:

based on:

git:
  branchPrefix: 'ruudk/{{ formatCurrentDate "2006/1" }}/'

[stefanhaller]

Ok, now I understand better what you are trying to do.

I do have to wonder if it would be better to expose a general runCommand (or exec or some such) function that would allow you to do initialValue: ruudk/{{ runCommand "date +'%Y/%-m'" }}/. Isn't that a lot more flexible than exposing bespoke functions for whatever thing users want to put in their branch names?

[ruudk]

That would be more flexible for sure! Do you think it's hard to do? I can give it a try if you steer me in the right direction

[stefanhaller]

I don't think it would be hard at all; the only tricky thing is to decide how to handle errors. And maybe whether it's a problem if the command returns multi-line output; we could consider replacing line feeds with spaces (like echo does), or at least trim trailing newlines.

Anyway, something like this in your template.FuncMap should work (haven't tried it though):

	"runCommand": func(command string) string {
		output, err := self.c.Git().Custom.RunWithOutput(command)
		if err != nil {
			return err.Error() // is this the best way to handle errors?
		}
		return output

[ruudk]

It's working 🎉

branchPrefix: 'ruudk/{{ runCommand "date +\"%Y/%-m\"" }}/'
initialValue: 'ruudk/{{ runCommand "date +\"%Y/%-m\"" }}/'

[stefanhaller]

Nice! Now to your question about tests and documentation. It's true that the quote function has neither tests nor documentation. I'm not sure it needs tests, but adding a bit of documentation would be nice; this could simply go somewhere in Custom_Command_Keybindings.md (it's used in a few examples there). Then you can add documentation for the runCommand function next to it.

As for tests: yes, it would be good to have some for your new function, and I think integration tests would be best. For custom commands you could use pkg/integration/tests/custom_commands/selected_path.go as an example, yours would probably be even simpler. For the branch prefix you could take pkg/integration/tests/commit/new_branch_with_prefix.go as an example; for some reason this is in tests/commit which doesn't make a lot of sense, you could move this to tests/branch as a cleanup commit first.

Also, can you please squash the commits, the commit history is not reviewable like this.

[ruudk]

Thanks for the guidance @stefanhaller, I'll follow up later with the changes.

[ruudk]

@stefanhaller I applied your feedback and added the docs and tests. Could you approve the workflow so we can see if it works correctly? Thanks!

[stefanhaller]

Very nice; I pushed a few fixups, I sometimes find this easier than explaining what I would like to see changed. (In one case I did both now though. 😄)

Feel free to squash them if you agree.

The final thing to do would be to update the PR description, it is out of date with what we are doing here now. This is important because it will end up in the commit message of the merge commit.

[ruudk]

@stefanhaller Thank you! I updated the PR description and fixed up all the commits.

[ruudk]

@stefanhaller You did a rebase? Let's see if it's still green.

[stefanhaller]

I only fixed the subject of the first commit, and the description of one of the tests. Shouldn't have changed anything.

[stefanhaller]

I'll wait with merging until @jesseduffield had a chance to look at the approach we're taking here (just in case), and also to see if the folks over in #4183 have anything to say about it. They were trying to solve the same problem there, but with a different approach.

[ruudk]

@jesseduffield I'm curious to hear your opinion about this. Are there things that I should change?

[jesseduffield]

LGTM

[jesseduffield]

Does this need to be declared separately?

[stefanhaller]

Yes, I think so. Without this you'd have to say := below, but that makes suggestedBranchName a new variable in that scope, so that doesn't work.

[jesseduffield]

ah yes, makes sense