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

style: orm module #15610

Closed
wants to merge 16 commits into from
Closed

style: orm module #15610

wants to merge 16 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Mar 30, 2023 •
edited
Loading

Description

This PR works towards #15546

Per feedback, I've kept this to a single module -- in fact, just to make things simpler, I am going to remove the changes to all folders other than orm.

So, this lints just ORM, which is great. I've always found it confusing.

I think that some of the changes here to names of functions really do make it less confusing.

Lint Merge Order guide:

This will make reviews easier as following this merge order will recuce the size of diffs.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@faddat faddat marked this pull request as ready for review March 30, 2023 02:56
@faddat faddat requested a review from a team as a code owner March 30, 2023 02:56
@github-prbot github-prbot requested review from a team, facundomedica and tac0turtle and removed request for a team March 30, 2023 02:56
@faddat faddat closed this Mar 30, 2023
@faddat faddat reopened this Mar 30, 2023
@github-prbot github-prbot requested a review from a team March 30, 2023 03:01
@faddat faddat changed the title orm module Style: orm module Mar 30, 2023
@faddat faddat changed the title Style: orm module style: orm module Mar 30, 2023
@faddat faddat mentioned this pull request Mar 30, 2023
Merged
19 tasks
julienrbrt
julienrbrt reviewed Mar 30, 2023
View reviewed changes
.golangci.yml Outdated Show resolved Hide resolved
tac0turtle
tac0turtle reviewed Mar 30, 2023
View reviewed changes
.golangci.yml Outdated Show resolved Hide resolved
@faddat
Copy link
Contributor Author

faddat commented Mar 30, 2023

done

@faddat
Copy link
Contributor Author

faddat commented Mar 30, 2023 •
edited
Loading

I need to reduce this to orm-only again

....done

@mergify mergify bot mentioned this pull request Mar 30, 2023
Closed
19 tasks
julienrbrt
julienrbrt reviewed Mar 30, 2023
View reviewed changes
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

looks in general good but I think a few things should be reverted:

  • the codegen generating any instead of interfaces
  • those arguments swapping.
  • those methods renaming

orm/model/ormtable/auto_increment_test.go

golden.Assert(t, debugBuf.String(), "test_auto_inc.golden")
checkEncodeDecodeEntries(t, table, store.IndexStoreReader())
}

func runAutoIncrementScenario(t *testing.T, table ormtable.AutoIncrementTable, ctx context.Context) {
// runAutoIncrementScenario runs a simple scenario with an auto-increment table.
func runAutoIncrementScenario(ctx context.Context, t *testing.T, table ormtable.AutoIncrementTable) {
Copy link
Member

Choose a reason for hiding this comment

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

I am not for swapping argument of functions in style PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this complies with a linter rule, actually two of them:

  • revive: context-as-argument
  • thelper

orm/encoding/ormfield/codec_test.go
@@ -99,7 +97,7 @@ func TestCompactUInt32(t *testing.T) {
testEncodeDecode(1073741823, 4)
testEncodeDecode(1073741824, 5)

// randomized tests
// Randomized tests.
Copy link
Member

Choose a reason for hiding this comment

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

Curious, is it a linter doing this? If so, which one? I think this has a too big impact to run such linter in the whole SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I have disabled it. Also I agree. Let me poke about on that.

orm/internal/codegen/codegen.go
@@ -4,12 +4,11 @@ import (
"fmt"
"os"

ormv1 "cosmossdk.io/api/cosmos/orm/v1"
Copy link
Member

Choose a reason for hiding this comment

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

we should keep the import group

orm/internal/codegen/file.go
@@ -66,18 +66,14 @@ func (f fileGen) genStoreInterface(stores []*protogen.Message) {
}

func (f fileGen) genStoreStruct(stores []*protogen.Message) {
// struct
// Struct.
Copy link
Member

Choose a reason for hiding this comment

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

yep, I don't like that comment changing linter 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok let me hunt it please.

orm/internal/codegen/index.go
@@ -5,19 +5,18 @@ import (
"strings"

"github.com/iancoleman/strcase"
"google.golang.org/protobuf/reflect/protoreflect"
)

func (t tableGen) genIndexKeys() {
Copy link
Member

Choose a reason for hiding this comment

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

Let's not change anything in the codegen in a linting PR, please.
Changing it to any makes the generated code require a specific minimum go version, while it wasn't the case before.

orm/internal/codegen/query.go
func (w *writer) F(format string, args ...interface{}) {
_, err := w.Write([]byte(w.indentStr))
func (w *writer) F(format string, args ...any) {
_, err := w.WriteString(w.indentStr)
Copy link
Member

Choose a reason for hiding this comment

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

👍🏾

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revive/use-any

orm/internal/codegen/table.go
"github.com/cosmos/cosmos-sdk/orm/internal/fieldnames"
"github.com/cosmos/cosmos-sdk/orm/model/ormtable"
"google.golang.org/protobuf/compiler/protogen"
Copy link
Member

Choose a reason for hiding this comment

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

the grouping was fine.

orm/orm.go
"github.com/cosmos/cosmos-sdk/orm/model/ormdb"
"github.com/cosmos/cosmos-sdk/orm/model/ormtable"
"google.golang.org/protobuf/proto"
Copy link
Member

Choose a reason for hiding this comment

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

grouping to be reverted.

orm/model/ormtable/primary_key.go
}

func (p primaryKeyIndex) get(backend ReadBackend, message proto.Message, values []protoreflect.Value) (found bool, err error) {
func (p primaryKeyIndex) doGet(backend ReadBackend, message proto.Message, values []protoreflect.Value) (found bool, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

To be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revive/confusing-naming

@faddat
Copy link
Contributor Author

faddat commented Mar 30, 2023

Since this one seems to need some tlc, I will approach it again tomorrow

@faddat faddat mentioned this pull request Mar 30, 2023
Closed
19 tasks
@faddat faddat marked this pull request as draft March 30, 2023 17:03
@faddat faddat closed this Mar 30, 2023
@julienrbrt julienrbrt mentioned this pull request Mar 30, 2023
Closed
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julienrbrt julienrbrt julienrbrt left review comments

@tac0turtle tac0turtle tac0turtle left review comments

@facundomedica facundomedica Awaiting requested review from facundomedica

@testinginprod testinginprod Awaiting requested review from testinginprod testinginprod will be requested when the pull request is marked ready for review testinginprod is a code owner

@raynaudoe raynaudoe Awaiting requested review from raynaudoe raynaudoe will be requested when the pull request is marked ready for review raynaudoe is a code owner

@lucaslopezf lucaslopezf Awaiting requested review from lucaslopezf lucaslopezf will be requested when the pull request is marked ready for review lucaslopezf is a code owner

@kocubinski kocubinski Awaiting requested review from kocubinski kocubinski will be requested when the pull request is marked ready for review kocubinski is a code owner

@akhilkumarpilli akhilkumarpilli Awaiting requested review from akhilkumarpilli akhilkumarpilli will be requested when the pull request is marked ready for review akhilkumarpilli is a code owner

@sontrinh16 sontrinh16 Awaiting requested review from sontrinh16 sontrinh16 will be requested when the pull request is marked ready for review sontrinh16 is a code owner

@aaronc aaronc Awaiting requested review from aaronc aaronc will be requested when the pull request is marked ready for review aaronc is a code owner

Assignees

@facundomedica facundomedica

@tac0turtle tac0turtle

@julienrbrt julienrbrt

Labels
C:CLI C:Keys Keybase, KMS and HSMs C:orm C:Simulations C:x/auth C:x/authz C:x/bank C:x/distribution distribution module related C:x/genutil genutil module issues C:x/gov C:x/group C:x/mint C:x/slashing C:x/staking Type: Build Type: CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@faddat @tac0turtle @julienrbrt @facundomedica