Implement invalid GITHUB_TOKEN handling #34
Conversation
chshersh
added
type: feature
chshersh
left a comment
chshersh
left a comment
There was a problem hiding this comment.
Great stuff! I see that you've put care into writing this code and did lots of good refactorings.
I left a few suggestions to improve the code even further but the general idea is correct 🙂
lib/gh/client.ml
Outdated
| ~url:github_api_url () | ||
| in | ||
| match response with | ||
| | Error (_code, msg) -> Ok (Printf.sprintf "Error: %s" msg) |
There was a problem hiding this comment.
Let's add one more constructor Curl_error of { code: int; msg: string } to error and return it here.
I don't think it should affect much of the rest code.
|
|
||
| (** Query all open issues *) | ||
| val issues : owner:string -> repo:string -> t list | ||
| val issues : owner:string -> repo:string -> (t list, Client.error) result |
There was a problem hiding this comment.
💯
This is a nice change of type signature!
| type t = { | ||
| pull_requests : Gh.Pr.t list lazy_t; | ||
| error : Gh.Client.error option; | ||
| } | ||
|
|
||
| let make ~owner ~repo = | ||
| let pull_requests, error = | ||
| match Gh.Pr.pull_requests ~owner ~repo with | ||
| | Ok issues -> (lazy issues, None) | ||
| | Error err -> (lazy [], Some err) | ||
| in | ||
| { pull_requests; error } |
There was a problem hiding this comment.
Thanks for creating this as a separate file! This was the plan anyway 🙂
lib/tui/widget/issue.ml
Outdated
| let issues = | ||
| match issues_tab.error with | ||
| | None -> fmt_issues ~selected:issues_tab.offset issues_tab.issues | ||
| | Some No_github_token -> Doc.str "\u{26A0} GITHUB_TOKEN not found" |
There was a problem hiding this comment.
Curious, what is this symbol? 👀 Do you have a screenshot?
There was a problem hiding this comment.
Also, let's make the message more helpful. Let's do something like:
\u{26A0} GITHUB_TOKEN not found. Make sure it's configured in your environment.
If you don't have a token, visit the following page to create one:
• https://github.com/settings/tokens
There was a problem hiding this comment.
I don't have nerd fonts installed on my machine, but it's supposed to be a warning sign like the one in terminal size warning.
lib/tui/widget/issue.ml
Outdated
| let issues = | ||
| match issues_tab.error with | ||
| | None -> fmt_issues ~selected:issues_tab.offset issues_tab.issues | ||
| | Some No_github_token -> Doc.str "\u{26A0} GITHUB_TOKEN not found" | ||
| in | ||
| Doc.(vertical [ fmt_filters issues_tab.filter; str ""; issues ]) |
There was a problem hiding this comment.
Let's rewrite this view a bit differently: don't display issue selector at all if there're errors.
So, the idea is to pattern match on the error and if it's None, use the previous formatting, otherwise, use the new doc.
chshersh
left a comment
chshersh
left a comment
There was a problem hiding this comment.
Looks great!
There's some duplication, but it can be handled separately.
Looks like the formatted is not happy. Once it's fixed, I'm happy to merge 👍🏻
2 participants
closes #23