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

Numeric conversion cleanup #7465

Merged
merged 38 commits into from
Oct 13, 2020
Merged

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Oct 8, 2020

What type of PR is this?

Cleanup

What does this PR do? Why is it needed?

Simplify/organize numeric conversions.

  • uint64()
  • int64()
  • uint()
  • int()

Which issues(s) does this PR fix?

N/A

Other notes for review

N/A

@rkapka rkapka changed the title validator count Numerical conversion cleanup Oct 8, 2020
@rkapka rkapka changed the title Numerical conversion cleanup Numeric conversion cleanup Oct 8, 2020
rkapka added 15 commits October 8, 2020 14:32
This reverts commit 19b376e.

# Conflicts:
#	beacon-chain/rpc/beacon/validators.go
This reverts commit f4acd6e.
This reverts commit 2a5c9ee.
This reverts commit 2863f9c.
This reverts commit 6e8385f.
This reverts commit c64a153.
This reverts commit 7a0931c.
# Conflicts:
#	beacon-chain/powchain/log_processing.go
This reverts commit 8b34137.
@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #7465 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #7465   +/-   ##
=======================================
  Coverage   60.74%   60.74%           
=======================================
  Files         427      427           
  Lines       30385    30385           
=======================================
  Hits        18458    18458           
  Misses       8940     8940           
  Partials     2987     2987           

@rkapka rkapka marked this pull request as ready for review October 9, 2020 13:15
@rkapka rkapka requested a review from a team as a code owner October 9, 2020 13:15
@prestonvanloon
Copy link
Member

Deepsource is still complaining that there are new issues introduced. Can you please take a look?

@@ -722,6 +722,6 @@ func (s *Service) logTillChainStart() {

log.WithFields(logrus.Fields{
"Extra validators needed": valNeeded,
"Time till minimum genesis": fmt.Sprintf("%s", time.Duration(secondsLeft)*time.Second),
"Time till minimum genesis": fmt.Sprint((time.Duration(secondsLeft) * time.Second).String()),
Copy link
Member

Choose a reason for hiding this comment

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

No need for fmt.Sprint?

@rkapka
Copy link
Contributor Author

rkapka commented Oct 9, 2020

Deepsource is still complaining that there are new issues introduced. Can you please take a look?

I don't think these are really new issues. Maybe nobody has touched these packages since we introduced DeepSource into the pipeline? Anyway, the only issues left are:

  1. deprecated usage
  2. plain channel

For (1) we have #6891. As for (2), this was already discussed on Discord and this "anti-pattern" seems to be a fairly standard Go pattern. I spoke about this with Nishant and Victor some time ago. We should confirm this and maybe configure the tool to ignore it?

prestonvanloon
prestonvanloon previously approved these changes Oct 12, 2020
# Conflicts:
#	validator/keymanager/v2/derived/deposit_test.go
@rkapka rkapka requested a review from prestonvanloon October 13, 2020 10:58
@prylabs-bulldozer prylabs-bulldozer bot merged commit b742511 into master Oct 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the numeric-conversion-cleanup branch October 13, 2020 12:43
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