Handle missing GITHUB_TOKEN

[chshersh]

Currently, github-tui will fail at runtime with the Not_found exception if the GITHUB_TOKEN environment variable is not present:

github-tui/lib/gh/client.ml

Lines 3 to 4 in f71814e

	let query query_body =
	let token = Sys.getenv "GITHUB_TOKEN" in

This is bad UX, so let's do the following:

1. Introduce a new error type error inside the Client module with one constructor No_github_token
2. Change the type of query to return (string, error) result
3. Use Sys.getenv_opt to get option instead of exception

[loadre]

Hi! :) I'm trying to look into this issue, and this is probably the first module to have its own error type, so I couldn't find much reference in terms of graceful error handling. The closest one might be in tui/widget/code.ml using failwith, but I think it's more of an "unreachable" kind of scenario.

Currently I'm thinking functions that use Client.query can short circuit for now and return an empty t list. But there has to be a better way.

[chshersh]

Hi @loadre 👋🏻

Indeed, this is the first place where errors are introduced but more errors will be added later. We'll deal with them later as well.

I agree that returning an empty list of issues is bad UX, since the user won't know there's an error.

I propose the following plan:

1. Functions Gh.Issue.issues and Gh.Pr.pull_requests will return result with error as well.
2. The Issue.t model type will have one more field with a lazy error:

   • github-tui/lib/tui/model/issue.mli

     Lines 9 to 14 in 9c07624

     	type t = {
     	all_issues : Gh.Issue.t list Lazy.t;
     	filter : filter;
     	issues : Gh.Issue.t list Lazy.t;
     	offset : int;
     	}

3. When fetching issues, the error from Gh.Issue.issues will be checked and populated here:

   • github-tui/lib/tui/model/issue.ml

     Lines 32 to 37 in 9c07624

     	let make ~owner ~repo =
     	let all_issues = lazy (Gh.Issue.issues ~owner ~repo) in
     	let filter = filter_open in
     	let issues = lazy_filter_issues ~filter all_issues in
     	let offset = 0 in
     	{ all_issues; filter; issues; offset }

4. When rendering the issue, the error field must be checked first and the error must be rendered instead of an empty list of issues

   • github-tui/lib/tui/widget/issue.ml

     Lines 86 to 93 in 9c07624

     	let section (issues_tab : Model.Issue.t) =
     	Doc.(
     	vertical
     	[
     	fmt_filters issues_tab.filter;
     	str "";
     	fmt_issues ~selected:issues_tab.offset issues_tab.issues;
     	])

Ideally, we want the same for PRs.

[loadre]

Sounds good! On a side note, I wonder if it's a better option to change Model.Issue.t into something like

type t = 
| Err of Gh.Client.error 
| Issues of { ... }

because it's unlikely that we get a list of issues alongside the client error. This could help get rid of invalid states down the road, but it would require further rewrite of functions that accept a Model.Issue.t.

[chshersh]

@loadre I thought about this, and usually I love making illegal states unrepresentable.

However, in this case, I feel like it'll introduce a lot of accidental complexity.

First, functions like filtering of issues will become more complex

github-tui/lib/tui/model/issue.ml

Lines 16 to 23 in 9c07624

	let filter_issues ~filter issues =
	match filter with
	| All -> issues
	| State state ->
	issues |> List.filter (fun (issue : Gh.Issue.t) -> issue.state = state)
	let lazy_filter_issues ~filter issues =
	lazy (issues |> Lazy.force |> filter_issues ~filter)

And second, I won't be able to change offset that easily.

github-tui/lib/tui/update.ml

Lines 7 to 10 in 9c07624

	let move_issues move (issues_tab : Model.Issue.t) =
	let len = issues_tab.issues |> Lazy.force |> List.length in
	let offset = (issues_tab.offset + move + len) mod len in
	{ issues_tab with offset }

In my view, this is a fine trade-off because there's only one place where we need to check for errors, and thus, the rest of the code doesn't need to care about it. Ensuring no invalid states will become quite invasive here.