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

feat!: enable golangci-lint #208

Merged
merged 5 commits into from
Jul 4, 2023
Merged

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Jul 3, 2023

Closes #204

Marked as breaking b/c the function signature for exported functions (RowRoots and ColRoots) was modified to include an error return parameter.

Testing

  1. golangci-lint run passes locally
  2. golangci-lint run passes in CI here

@rootulp rootulp self-assigned this Jul 3, 2023
@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Merging #208 (2b4078b) into master (ed26fa9) will decrease coverage by 3.50%.
The diff coverage is 56.89%.

❗ Current head 2b4078b differs from pull request most recent head 2939010. Consider uploading reports for the commit 2939010 to get more accurate results

@@            Coverage Diff             @@
##           master     #208      +/-   ##
==========================================
- Coverage   80.93%   77.43%   -3.50%     
==========================================
  Files           7        7              
  Lines         493      523      +30     
==========================================
+ Hits          399      405       +6     
- Misses         56       69      +13     
- Partials       38       49      +11     
Impacted Files Coverage Δ
datasquare.go 90.58% <43.75%> (-4.48%) ⬇️
extendeddatacrossword.go 67.17% <45.45%> (-3.78%) ⬇️
extendeddatasquare.go 71.13% <66.66%> (-2.50%) ⬇️
leopard.go 77.77% <100.00%> (ø)
tree.go 100.00% <100.00%> (ø)

Comment on lines +6 to +12
enable:
- exportloopref
- gofumpt
- misspell
- nakedret
- revive
- prealloc
Copy link
Collaborator Author

@rootulp rootulp Jul 4, 2023

Choose a reason for hiding this comment

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

copied the celestia-app .golanci.yml b/c it seems like a good starting point / uncontroversial

@@ -178,7 +178,13 @@ func (eds *ExtendedDataSquare) solveCrosswordRow(

// Insert rebuilt shares into square.
for c, s := range rebuiltShares {
eds.SetCell(uint(r), uint(c), s)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

previously this was silently ignoring an error! Thanks to errcheck, we are forced to check the error return type and propagate it which resulted in:

--- FAIL: TestRepairExtendedDataSquare (0.00s)
    --- FAIL: TestRepairExtendedDataSquare/leopard (0.00s)
        --- FAIL: TestRepairExtendedDataSquare/leopard/MaximumErasures (0.00s)
            /Users/rootulp/git/rootulp/celestia/rsmt2d/extendeddatacrossword_test.go:60: unexpected err while repairing data square: cannot set cell (3, 2) as it already has a value 05050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505, codec: :leopard

To resolve, 2b4078b checks if the cellToSet is nil prior to the attempt to SetCell

- release/**

env:
GO_VERSION: '1.20'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

inconsistent until #211 merges

@rootulp rootulp marked this pull request as ready for review July 4, 2023 15:28
@@ -196,8 +196,12 @@ func (eds *ExtendedDataSquare) Col(y uint) [][]byte {
}

// ColRoots returns the Merkle roots of all the columns in the square.
func (eds *ExtendedDataSquare) ColRoots() [][]byte {
return deepCopy(eds.getColRoots())
func (eds *ExtendedDataSquare) ColRoots() ([][]byte, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[note for reviewers] this is public API breaking

@@ -207,8 +211,12 @@ func (eds *ExtendedDataSquare) Row(x uint) [][]byte {
}

// RowRoots returns the Merkle roots of all the rows in the square.
func (eds *ExtendedDataSquare) RowRoots() [][]byte {
return deepCopy(eds.getRowRoots())
func (eds *ExtendedDataSquare) RowRoots() ([][]byte, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[note for reviewers] this is public API breaking

Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

LGTM!

datasquare_test.go Show resolved Hide resolved
datasquare_test.go Show resolved Hide resolved
@rootulp rootulp merged commit e8e042f into celestiaorg:master Jul 4, 2023
@rootulp rootulp deleted the rp/golangci-lint branch July 4, 2023 20:09
rootulp added a commit that referenced this pull request Jul 5, 2023

Run benchmarks
1. [Install Go](https://go.dev/doc/install) 1.19+
Copy link
Collaborator Author

@rootulp rootulp Jul 5, 2023

Choose a reason for hiding this comment

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

🤦‍♂️ this line is incorrect after I created #211

It should've said 1.20+

rootulp added a commit that referenced this pull request Jul 5, 2023
0xchainlover pushed a commit to celestia-org/rsmt2d that referenced this pull request Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable errcheck linter
3 participants