Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

introduce static analysis #265

Merged
merged 7 commits into from
Jan 8, 2022
Merged

Conversation

jkralik
Copy link
Member

@jkralik jkralik commented Dec 5, 2021

No description provided.

@jkralik jkralik force-pushed the jkralik/feature/staticAnalysis branch 2 times, most recently from 0207a1e to 2027c84 Compare December 5, 2021 09:08
@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2021

Codecov Report

Merging #265 (0fce43f) into master (9ead1a9) will decrease coverage by 0.11%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #265      +/-   ##
==========================================
- Coverage   63.68%   63.57%   -0.12%     
==========================================
  Files          79       79              
  Lines        5640     5655      +15     
==========================================
+ Hits         3592     3595       +3     
- Misses       1689     1697       +8     
- Partials      359      363       +4     
Impacted Files Coverage Δ
examples/mcast/client/main.go 0.00% <0.00%> (ø)
examples/observe/server/main.go 0.00% <0.00%> (ø)
message/message.go 0.00% <ø> (ø)
message/noresponse/noresponse.go 94.73% <ø> (ø)
tcp/clientconn.go 69.40% <ø> (ø)
udp/client/clientconn.go 72.98% <0.00%> (ø)
udp/client/mutexmap.go 87.50% <ø> (ø)
message/pool/message.go 59.00% <13.33%> (-3.77%) ⬇️
net/blockwise/blockwise.go 68.72% <40.00%> (-0.28%) ⬇️
tcp/clientobserve.go 70.58% <100.00%> (ø)
... and 3 more

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 9ead1a9...0fce43f. Read the comment docs.

@JosefWN
Copy link
Contributor

JosefWN commented Dec 10, 2021

Tip, you can manage a whole host of linters easily and efficiently using: https://github.com/golangci/golangci-lint

@Danielius1922
Copy link
Member

Tip, you can manage a whole host of linters easily and efficiently using: https://github.com/golangci/golangci-lint

Indeed, thanks, we have it included in the plgd-dev/hub repository. I've added there both stand-alone analysis and golangsci-lint, though I no longer recall why. (The latest released version and the version of the linter bundled in golangsci might be different? Can't remember, lets just say that I'm a linting maximalist.)

@@ -75,6 +76,9 @@ func (r *Message) ResetOptionsTo(in message.Options) {
r.valueBuffer = append(r.valueBuffer, make([]byte, used)...)
opts, used, err = r.msg.Options.ResetOptionsTo(r.valueBuffer, in)
}
if err != nil {
panic(fmt.Errorf("unexpected error: %w", err))
Copy link
Member

Choose a reason for hiding this comment

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

@jkralik panicking in these setters feels wrong. But what else can we do without changing the API? If the first call r.msg.Options.ResetOptionsTo gives us correct required size for the value buffer this should never occur. But any ideas how else might we enforce it?

@Danielius1922 Danielius1922 marked this pull request as ready for review January 8, 2022 16:02
@sonarcloud
Copy link

sonarcloud bot commented Jan 8, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

63.6% 63.6% Coverage
2.6% 2.6% Duplication

@Danielius1922 Danielius1922 merged commit 5a49c43 into master Jan 8, 2022
@Danielius1922 Danielius1922 deleted the jkralik/feature/staticAnalysis branch January 8, 2022 16:25
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.

None yet

4 participants