Skip to content

Adds new output format for sites/site-machines.json#146

Merged
nkinkade merged 5 commits intomasterfrom
sandbox-kinkade
Jun 27, 2022
Merged

Adds new output format for sites/site-machines.json#146
nkinkade merged 5 commits intomasterfrom
sandbox-kinkade

Conversation

@nkinkade
Copy link
Copy Markdown
Contributor

@nkinkade nkinkade commented Jun 27, 2022

This change is Reviewable

@nkinkade nkinkade requested review from cristinaleonr and robertodauria and removed request for robertodauria June 27, 2022 17:40
Copy link
Copy Markdown
Contributor

@cristinaleonr cristinaleonr left a comment

Choose a reason for hiding this comment

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

Is it possible to add a unit test like we have for the other formats?

Reviewable status: 0 of 1 LGTMs obtained

@coveralls
Copy link
Copy Markdown
Collaborator

coveralls commented Jun 27, 2022

Pull Request Test Coverage Report for Build 1147

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 94.182%

Totals Coverage Status
Change from base Build 1134: 0.03%
Covered Lines: 2056
Relevant Lines: 2183

💛 - Coveralls

Copy link
Copy Markdown
Contributor Author

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

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

@cristinaleonr: It is! And I have. PTAL?

Reviewable status: 0 of 1 LGTMs obtained

Copy link
Copy Markdown
Contributor

@cristinaleonr cristinaleonr left a comment

Choose a reason for hiding this comment

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

Great! I believe you forgot to push the "testdata/site-machines.json" test file, though.

Reviewable status: 0 of 1 LGTMs obtained


siteinfo/client_test.go line 274 at r2 (raw file):

	}

	// Test working HTTP client request

Nit: missing period.

Copy link
Copy Markdown
Contributor Author

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

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

Bah! I forgot to add the untracked file. I've added it now, and the build is working. PTAL?

Reviewable status: 0 of 1 LGTMs obtained


siteinfo/client_test.go line 274 at r2 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Nit: missing period.

Period added.

Copy link
Copy Markdown
Contributor

@cristinaleonr cristinaleonr left a comment

Choose a reason for hiding this comment

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

One more comment.
:lgtm:

Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained


siteinfo/client_test.go line 285 at r4 (raw file):

	_, err = client.SiteMachines()
	if err != nil {
		t.Error("SiteMachines(): expected success, got error.")

This is the one place we don't use Errorf, but it might be useful to add err to the error string.

Also, opposite nit: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

Copy link
Copy Markdown
Contributor

@cristinaleonr cristinaleonr left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained


siteinfo/client_test.go line 285 at r4 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

This is the one place we don't use Errorf, but it might be useful to add err to the error string.

Also, opposite nit: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

Actually I just saw the nit does not apply to print statements :). Feel free to ignore.

Copy link
Copy Markdown
Contributor Author

@nkinkade nkinkade left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained


siteinfo/client_test.go line 285 at r4 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Actually I just saw the nit does not apply to print statements :). Feel free to ignore.

This test now uses t.Errorf() and should display the error. Additionally, I noticed that the other uses of t.Errorf() were unnecessary, as none of the statements actually had any formatting strings. I'm surprised that the Go linter in vscode didn't point that out to me. In any case, I have changed those to just use t.Error().

@nkinkade nkinkade merged commit 63f1491 into master Jun 27, 2022
@nkinkade nkinkade deleted the sandbox-kinkade branch June 27, 2022 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants