Skip to content
This repository was archived by the owner on Aug 24, 2022. It is now read-only.

Add exhaustiveness checks on our "sum types". - #45

Merged
AlekSi merged 8 commits into
masterfrom
sumtype
May 7, 2019
Merged

Add exhaustiveness checks on our "sum types".#45
AlekSi merged 8 commits into
masterfrom
sumtype

Conversation

@AlekSi

@AlekSi AlekSi commented May 6, 2019

Copy link
Copy Markdown
Contributor

@AlekSi AlekSi self-assigned this May 6, 2019
Comment thread agentlocal/agent_local_test.go Outdated
@codecov

codecov Bot commented May 7, 2019

Copy link
Copy Markdown

Codecov Report

Merging #45 into master will increase coverage by 0.18%.
The diff coverage is 38.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
+ Coverage   50.45%   50.64%   +0.18%     
==========================================
  Files          24       24              
  Lines        2949     2952       +3     
==========================================
+ Hits         1488     1495       +7     
+ Misses       1372     1368       -4     
  Partials       89       89
Impacted Files Coverage Δ
client/client.go 27.89% <0%> (+1.47%) ⬆️
client/channel/channel.go 83.64% <69.69%> (-4.47%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9714f0...e1e4e09. Read the comment docs.

@AlekSi AlekSi changed the title Sumtype Add exhaustiveness checks on our "sum types". May 7, 2019
@AlekSi
AlekSi marked this pull request as ready for review May 7, 2019 06:11
@AlekSi
AlekSi requested review from BupycHuk and idexter May 7, 2019 06:11
Comment thread Makefile
check-license: ## Check that all files have the same license header.
check: ## Run required checkers and linters.
go run .github/check-license.go
go-sumtype ./vendor/... ./...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why don't we run golangci-lint here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

https://golangci.com runs it on CI.
During development, the most typical use is golangci-lint run which is not much longer than make check. But another common use-case is golangci-lint run --new-from-rev=PMM-2.0 to check only new/changed code. I kind of want to promote both without introducing three Makefile targets (the third one for Travis CI that does not run golangci-lint).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Another option is

check-travis:
  go run .github/check-license.go
  go-sumtype ./vendor/... ./...

check: check-travis
  golangci-lint run

But that leaves out golangci-lint run --new-from-rev=PMM-2.0.

Comment thread client/channel/channel.go Outdated
// Request represents an request from server.
// It is similar to agentpb.ServerMessage except it can contain only requests,
// and the payload is already unwrapped (XXX instead of ServerMessage_XXX).
type Request struct {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe it's better to rename these two types to ServerRequest and AgentResponse, because agent can send request to pmm-managed as well.

@AlekSi
AlekSi merged commit 791d5e2 into master May 7, 2019
@AlekSi
AlekSi deleted the sumtype branch May 7, 2019 14:09
isabek pushed a commit to isabek/pmm-agent that referenced this pull request Feb 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants