From 71b13a81d0ac8c7fe8589ec3aeff9fcafd9da10b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 21 Jul 2020 13:47:27 +0200 Subject: [PATCH 1/2] Correctly report HTTP and Markdown errors in `repo view` Any errors from fetching and rendering the README were silenced and ignored in `repo view`. This change: - Tolerates HTTP 404, but will raise exceptions for any other error; - Moves markdown rendering from `api` package to command implementation; - Ensures markdown rendering errors are correctly reported. --- api/queries_repo.go | 50 ++++++++++++++++++-------------------------- command/repo.go | 29 ++++++++++++++++++++++--- command/repo_test.go | 5 ++++- 3 files changed, 50 insertions(+), 34 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 3469ea80e..35079095c 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -12,7 +12,6 @@ import ( "time" "github.com/cli/cli/internal/ghrepo" - "github.com/cli/cli/utils" "github.com/shurcooL/githubv4" ) @@ -406,35 +405,35 @@ func RepoCreate(client *Client, input RepoCreateInput) (*Repository, error) { return initRepoHostname(&response.CreateRepository.Repository, "github.com"), nil } -func RepositoryReadme(client *Client, fullName string) (string, error) { - type readmeResponse struct { +type RepoReadme struct { + Filename string + Content string +} + +func RepositoryReadme(client *Client, repo ghrepo.Interface) (*RepoReadme, error) { + var response struct { Name string Content string } - var readme readmeResponse - - err := client.REST("GET", fmt.Sprintf("repos/%s/readme", fullName), nil, &readme) - if err != nil && !strings.HasSuffix(err.Error(), "'Not Found'") { - return "", fmt.Errorf("could not get readme for repo: %w", err) - } - - decoded, err := base64.StdEncoding.DecodeString(readme.Content) + err := client.REST("GET", fmt.Sprintf("repos/%s/readme", ghrepo.FullName(repo)), nil, &response) if err != nil { - return "", fmt.Errorf("failed to decode readme: %w", err) - } - - readmeContent := string(decoded) - - if isMarkdownFile(readme.Name) { - readmeContent, err = utils.RenderMarkdown(readmeContent) - if err != nil { - return "", fmt.Errorf("failed to render readme as markdown: %w", err) + var httpError HTTPError + if errors.As(err, &httpError) && httpError.StatusCode == 404 { + return nil, &NotFoundError{err} } + return nil, err } - return readmeContent, nil + decoded, err := base64.StdEncoding.DecodeString(response.Content) + if err != nil { + return nil, fmt.Errorf("failed to decode readme: %w", err) + } + return &RepoReadme{ + Filename: response.Name, + Content: string(decoded), + }, nil } type RepoMetadataResult struct { @@ -895,12 +894,3 @@ func RepoMilestones(client *Client, repo ghrepo.Interface) ([]RepoMilestone, err return milestones, nil } - -func isMarkdownFile(filename string) bool { - // kind of gross, but i'm assuming that 90% of the time the suffix will just be .md. it didn't - // seem worth executing a regex for this given that assumption. - return strings.HasSuffix(filename, ".md") || - strings.HasSuffix(filename, ".markdown") || - strings.HasSuffix(filename, ".mdown") || - strings.HasSuffix(filename, ".mkdown") -} diff --git a/command/repo.go b/command/repo.go index 9d4a1dc6b..e68a86bdd 100644 --- a/command/repo.go +++ b/command/repo.go @@ -1,6 +1,7 @@ package command import ( + "errors" "fmt" "net/url" "os" @@ -612,10 +613,23 @@ func repoView(cmd *cobra.Command, args []string) error { return err } - readmeContent, _ := api.RepositoryReadme(apiClient, fullName) + readme, err := api.RepositoryReadme(apiClient, toView) + var notFound *api.NotFoundError + if err != nil && !errors.As(err, ¬Found) { + return err + } - if readmeContent == "" { - readmeContent = utils.Gray("No README provided") + var readmeContent string + if readme == nil { + readmeContent = utils.Gray("This repository does not have a README") + } else if isMarkdownFile(readme.Filename) { + var err error + readmeContent, err = utils.RenderMarkdown(readme.Content) + if err != nil { + return fmt.Errorf("error rendering markdown: %w", err) + } + } else { + readmeContent = readme.Content } description := repo.Description @@ -648,3 +662,12 @@ func repoView(cmd *cobra.Command, args []string) error { func repoCredits(cmd *cobra.Command, args []string) error { return credits(cmd, args) } + +func isMarkdownFile(filename string) bool { + // kind of gross, but i'm assuming that 90% of the time the suffix will just be .md. it didn't + // seem worth executing a regex for this given that assumption. + return strings.HasSuffix(filename, ".md") || + strings.HasSuffix(filename, ".markdown") || + strings.HasSuffix(filename, ".mdown") || + strings.HasSuffix(filename, ".mkdown") +} diff --git a/command/repo_test.go b/command/repo_test.go index 56c52fa31..8d103645c 100644 --- a/command/repo_test.go +++ b/command/repo_test.go @@ -912,6 +912,9 @@ func TestRepoView_blanks(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query RepositoryInfo\b`), httpmock.StringResponse("{}")) + http.Register( + httpmock.REST("GET", "repos/OWNER/REPO/readme"), + httpmock.StatusStringResponse(404, `{}`)) output, err := RunCommand("repo view") if err != nil { @@ -921,6 +924,6 @@ func TestRepoView_blanks(t *testing.T) { test.ExpectLines(t, output.String(), "OWNER/REPO", "No description provided", - "No README provided", + "This repository does not have a README", "View this repository on GitHub: https://github.com/OWNER/REPO") } From 86eacc37892086a5e530eb83523fe5d988e05158 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 21 Jul 2020 13:59:05 +0200 Subject: [PATCH 2/2] Enable unwrapping `api.NotFoundError` --- api/queries_pr.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/api/queries_pr.go b/api/queries_pr.go index 46fa014cd..4437f8f7e 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -134,6 +134,10 @@ type NotFoundError struct { error } +func (err *NotFoundError) Unwrap() error { + return err.error +} + func (pr PullRequest) HeadLabel() string { if pr.IsCrossRepository { return fmt.Sprintf("%s:%s", pr.HeadRepositoryOwner.Login, pr.HeadRefName)