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.

Sign up for GitHub

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

Jump to bottom

Fix deepsource errors. #1789

Merged
merged 5 commits into from
Sep 17, 2021
Merged

Fix deepsource errors. #1789

merged 5 commits into from
Sep 17, 2021

Conversation

arijitAD
Copy link
Contributor

Changes

Tests

TestBlockAnnounceHandshakeSize

Issues

Primary Reviewer

@codecov
Copy link

codecov bot commented Sep 16, 2021 •
edited
Loading

Codecov Report

Merging #1789 (a14aeb1) into development (1d688e4) will decrease coverage by 0.02%.
The diff coverage is 91.89%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1789      +/-   ##
===============================================
- Coverage        59.28%   59.26%   -0.03%     
===============================================
  Files              186      186              
  Lines            20008    20012       +4     
===============================================
- Hits             11862    11860       -2     
- Misses            6088     6094       +6     
  Partials          2058     2058
Flag Coverage Δ
unit-tests 59.26% <91.89%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
dot/network/host.go 65.94% <80.00%> (+1.24%) ⬆️
dot/network/notifications.go 69.41% <90.00%> (+1.25%) ⬆️
dot/network/block_announce.go 73.95% <100.00%> (ø)
dot/network/service.go 69.56% <100.00%> (+0.24%) ⬆️
lib/grandpa/network.go 52.89% <100.00%> (ø)
dot/state/block_notify.go 73.86% <0.00%> (-9.10%) ⬇️
lib/babe/epoch.go 64.51% <0.00%> (-2.16%) ⬇️
dot/network/connmgr.go 79.46% <0.00%> (-0.90%) ⬇️
dot/digest/digest.go 64.62% <0.00%> (-0.88%) ⬇️
... and 1 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 1d688e4...a14aeb1. Read the comment docs.

@arijitAD arijitAD mentioned this pull request Sep 16, 2021
Merged
EclesioMeloJunior
EclesioMeloJunior approved these changes Sep 16, 2021
View reviewed changes
Copy link
Member

@EclesioMeloJunior EclesioMeloJunior left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm

noot
noot reviewed Sep 16, 2021
View reviewed changes
dot/network/host.go Outdated
@@ -225,18 +225,8 @@ func (h *host) close() error {
}

// registerStreamHandler registers the stream handler, appending the given sub-protocol to the main protocol ID
Copy link
Contributor

Choose a reason for hiding this comment

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

update comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

noot
noot reviewed Sep 16, 2021
View reviewed changes
dot/network/host.go
Comment on lines +244 to +247
var allNodes []peer.AddrInfo
allNodes = append(allNodes, h.bootnodes...)
allNodes = append(allNodes, h.persistentPeers...)
for _, addrInfo := range allNodes {
Copy link
Contributor

Choose a reason for hiding this comment

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

did deepsource complain about this? just curious what the issue was

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes https://deepsource.io/gh/ChainSafe/gossamer/run/7db3c103-c088-4fb5-904e-bad5ef591ab6/go/CRT-D0001

possibly assigns to a wrong variable

noot
noot approved these changes Sep 16, 2021
View reviewed changes
Copy link
Contributor

@noot noot left a comment

Choose a reason for hiding this comment

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

looks good!

@arijitAD arijitAD merged commit 4687cf5 into development Sep 17, 2021
@arijitAD arijitAD deleted the fix-deepsource-1 branch September 17, 2021 07:56
@github-actions
Copy link

github-actions bot commented Dec 3, 2021

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

timwu20 pushed a commit to timwu20/gossamer that referenced this pull request Dec 6, 2021
@arijitAD @timwu20
Fix deepsource errors. (ChainSafe#1789)
09ca34a 
* Remove control flag from the function definition.

* Remove method receiver from unreferenced methods.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noot noot noot approved these changes

@EclesioMeloJunior EclesioMeloJunior EclesioMeloJunior approved these changes

@edwardmack edwardmack Awaiting requested review from edwardmack

@jimjbrettj jimjbrettj Awaiting requested review from jimjbrettj jimjbrettj is a code owner

@timwu20 timwu20 Awaiting requested review from timwu20 timwu20 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arijitAD @EclesioMeloJunior @noot @dutterbutter