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.

Sign up for GitHub

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

Jump to bottom

Premature error fix for issue #353 #357

Closed
wants to merge 4 commits into from
Closed

Premature error fix for issue #353 #357

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9c02ccc
fix(keybinds): Address premature error re; #353
Omnikron13 Apr 17, 2024
e189492
style: capitalisation in comment
Omnikron13 Apr 18, 2024
bf2338e
fix(keybinds): replace struct with map for template inputs (#353)
Omnikron13 Apr 18, 2024
cce3e09
fix(keybinds): replace fatal error with bubbletea error message
Omnikron13 May 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 14 additions & 22 deletions ui/modelUtils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,6 @@ type IssueCommandTemplateInput struct {
HeadRefName string
}

type PRCommandTemplateInput struct {
RepoName string
RepoPath string
PrNumber int
HeadRefName string
BaseRefName string
}

func (m *Model) executeKeybinding(key string) tea.Cmd {
currRowData := m.getCurrRowData()

Expand Down Expand Up @@ -108,32 +100,32 @@ func (m *Model) executeKeybinding(key string) tea.Cmd {
}

func (m *Model) runCustomPRCommand(commandTemplate string, prData *data.PullRequestData) tea.Cmd {
repoName := prData.GetRepoNameWithOwner()
repoPath, ok := common.GetRepoLocalPath(repoName, m.ctx.Config.RepoPaths)

if !ok {
return func() tea.Msg {
return constants.ErrMsg{Err: fmt.Errorf("Failed to find local path for repo %s", repoName)}
}
// A generic map is a pretty easy & flexible way to populate a template if there's no pressing need
// for sructured data, existing structs, etc. Especially if holes in the data are expected.
input := map[string]any {
"RepoName": prData.GetRepoNameWithOwner(),
"PrNumber": prData.Number,
"HeadRefName": prData.HeadRefName,
"BaseRefName": prData.BaseRefName,
}

input := PRCommandTemplateInput{
RepoName: repoName,
RepoPath: repoPath,
PrNumber: prData.Number,
HeadRefName: prData.HeadRefName,
BaseRefName: prData.BaseRefName,
// Append in the local RepoPath only if it can be found
if repoPath, ok := common.GetRepoLocalPath(input["RepoName"].(string), m.ctx.Config.RepoPaths); ok {
input["RepoPath"] = repoPath
}

cmd, err := template.New("keybinding_command").Parse(commandTemplate)
if err != nil {
log.Fatal("Failed parse keybinding template", err)
}

// Set the command to error out if required input (e.g. RepoPath) is missing
cmd = cmd.Option("missingkey=error")
Copy link
Owner

Choose a reason for hiding this comment

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

Can we remove the log.Fatal on line 128 and instead return a tea.Cmd?
Use a more appropriate message though :)

		return func() tea.Msg {
			return constants.ErrMsg{Err: fmt.Errorf("Failed to find local path for repo %s", repoName)}
		}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, no I don't think that is the right approach...

Templates can be pretty complex. They can be recursively nested into a tree structure, be populated out in multiple stages by different calls to the Execute functions with different sources and types of data, potentially across multiple goroutines simultaneously.

The values passed in can be chained on one another (I presume to key into a map or struct which is another value passed in). They can be (zero-argument*) functions/methods that implements some kind of literal piping with | to produce the final substitution. I believe they may return a 2nd err return which will be propagated out ultimately. ( * there is a manner in which the final piece of data passed in, can have arguments, but honestly there's a lot going on in there, and a lot of ways and reasons it might spit errors at you. )

There's definitely cases where it will error on 'empty or incomplete' template, other internal/syntactic issues, unexpected data types or values, etc...

The Option that this branch sets is essentially stating;

'This is our final pass over this Template, and we will consider any dangling placeholders left unfilled as also being errors.

While trivial current uses seem in the source to suggest the other values passed in are reliable, future additions may not be. The writer of the template might typo key/member names, screw up their syntax, etc. and cause an unrelated error, or the template may have its own reasons to.

It could certainly complicate the error handing if you want to tease any nuance out, though I think you may be able to simple propagate the errors, or wrapped versions of them, up to wherever and how that gets communicated to the user.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure how this relates to the fact you can show a log to the user using the tea.Command instead of crashing the app with fatal.Log.

return func() tea.Msg { return constants.ErrMsg{Err: fmt.Errorf("Failed to parsetemplate %s", template)} }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, think I totally misread/missed the point there... I didn't put any of the log.Fatal()'s in, so I think my mind went elsewhere.

There's a few sprinkled about, though I confess I'm not really up to speed with bubbletea yet, so not sure I was even aware what that snippet was doing tbh. >_>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made that change, though #362 would supersede this PR if it is the design direction you favour.


var buff bytes.Buffer
err = cmd.Execute(&buff, input)
if err != nil {
log.Fatal("Failed executing keybinding command", err)
return func() tea.Msg { return constants.ErrMsg{Err: fmt.Errorf("Failed to parsetemplate %s", commandTemplate)} }
}
return m.executeCustomCommand(buff.String())
}
Expand Down