dlvhdr / gh-dash

A beautiful CLI dashboard for GitHub 🚀
https://dlvhdr.github.io/gh-dash
MIT License
6.77k stars 191 forks source link

Premature error fix for issue #353 #357

Closed Omnikron13 closed 1 month ago

Omnikron13 commented 2 months ago

As per #353, commands were failing due to unavailable data

How did you test this change?

Duplicate a hotkey command akin to that reported as failing to confirm. Grepped the source for elements I needed to remove/modify.

Ran go test suite, manually verified command now successfully executes.

Omnikron13 commented 2 months ago

With that said, I think there is perhaps a fair bit of quite aggressive reworking of that source file that is low-hanging fruit to improve flexibility and provide a broader framework for new features, screens etc.

I'll put a little work in on that in another branch, might make it easier to work out the ideal was to act here.

Omnikron13 commented 2 months ago

Applying the same fixes to the issues version of function things look pretty parse when you diff the two functions::

func (m *Model) runCustomIssueCommand(commandTemplate string, | func (m *Model) runCustomPRCommand(commandTemplate string, pr
    // A generic map is often the simplest way to pass a  |     // A generic map is a pretty easy & flexible way to p
                                 >      // for sructured data, existing structs, etc. Especia
    input := map[string]any {                   input := map[string]any {
        "RepoName":    issueData.GetRepoNameWithOwner |         "RepoName":    prData.GetRepoNameWithOwner(),
      "IssueNumber": issueData.Number,           |          "PrNumber":    prData.Number,
                                 >          "HeadRefName": prData.HeadRefName,
                                 >          "BaseRefName": prData.BaseRefName,
    }                               }

    // Append in the local RepoPath only if it can be fou       // Append in the local RepoPath only if it can be fou
    if repoPath, ok := common.GetRepoLocalPath(input["Rep       if repoPath, ok := common.GetRepoLocalPath(input["Rep
        input["RepoPath"] =  repoPath;           |          input["RepoPath"] = repoPath
    }                               }

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

    // Set the command to error out if required input (e.       // Set the command to error out if required input (e.
    cmd = cmd.Option("missingkey=error")                cmd = cmd.Option("missingkey=error")

    var buff bytes.Buffer                       var buff bytes.Buffer
    err = cmd.Execute(&buff, input)                 err = cmd.Execute(&buff, input)
    if err != nil {                         if err != nil {
        log.Fatal("Failed executing keybinding comman           log.Fatal("Failed executing keybinding comman
    }                               }
    return m.executeCustomCommand(buff.String())            return m.executeCustomCommand(buff.String())
}                               
Omnikron13 commented 1 month ago

I've split out common functionality as well in #362, if that is the direction you want to go.

dlvhdr commented 1 month ago

Thanks @Omnikron13! I merged the other PR, so feel free to finish any work/merge conflicts you have here.

Omnikron13 commented 1 month ago

If there's more refactoring to do, I think it depends a lot on what the future scope of the project is here. Is the plan to add screens for e.g. workflows, settings, wiki pages, etc. etc? Is there anywhere better to discuss the general project future & milestones than just in issues & PRs?

dlvhdr commented 1 month ago

You can always open discussions if you have something for it. I don't have any big plans right now