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

Refactor attribute validation #175

Merged
merged 13 commits into from
Mar 18, 2021
Merged

Conversation

channa-figure
Copy link
Contributor

@channa-figure channa-figure commented Mar 17, 2021

Description

Refactor existing Validate method in x/attribute/types/attribute.go to be ValidateBasic to follow sdk standard.

closes: #44

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented Mar 17, 2021

Codecov Report

Merging #175 (664e135) into main (f489d23) will decrease coverage by 0.35%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #175      +/-   ##
==========================================
- Coverage   46.06%   45.71%   -0.36%     
==========================================
  Files          96      102       +6     
  Lines        7858     8161     +303     
==========================================
+ Hits         3620     3731     +111     
- Misses       3830     4015     +185     
- Partials      408      415       +7     
Impacted Files Coverage Δ
x/attribute/types/genesis.go 0.00% <0.00%> (ø)
x/attribute/keeper/keeper.go 66.66% <87.50%> (ø)
x/attribute/keeper/genesis.go 23.07% <100.00%> (ø)
x/attribute/types/attribute.go 86.41% <100.00%> (+50.52%) ⬆️
x/attribute/types/msgs.go 19.56% <100.00%> (ø)
x/attribute/keeper/msg_server.go 2.27% <0.00%> (ø)
x/attribute/keeper/query_server.go 0.00% <0.00%> (ø)
x/attribute/keeper/params.go 100.00% <0.00%> (ø)
... and 2 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 f489d23...664e135. Read the comment docs.

@channa-figure channa-figure marked this pull request as ready for review March 18, 2021 16:05
@channa-figure channa-figure requested a review from a user March 18, 2021 16:05
ghost
ghost previously approved these changes Mar 18, 2021
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I guess this is fine (consistency in naming), but ValidateBasic is a sdk.Msgthing, not a general naming convention thing in Go.

Copy link
Member

@iramiller iramiller left a comment

Choose a reason for hiding this comment

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

We need to add the stateful check on attribute length based on the current configured max value length. The validation that occurs in the types folder must be stateless thus the "validatebasic" refactor...

x/attribute/keeper/keeper.go Show resolved Hide resolved
@iramiller
Copy link
Member

I guess this is fine (consistency in naming), but ValidateBasic is a sdk.Msgthing, not a general naming convention thing in Go.

Agreed ... the point of this issue originally was to address that the validation done in the attribute type could no longer be performed there due to the stateful property for the attribute length ... this validation has to be done in a keeper method. I think the point got lost a bit with the ValidateBasic focus @dpederson-figure

@ghost
Copy link

ghost commented Mar 18, 2021

It's cool. Just a nit.

iramiller
iramiller previously approved these changes Mar 18, 2021
Copy link
Member

@iramiller iramiller left a comment

Choose a reason for hiding this comment

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

Nice, LGTM.

@channa-figure channa-figure merged commit d6f6f0d into main Mar 18, 2021
@channa-figure channa-figure deleted the refactor-attribute-validation branch March 18, 2021 22:44
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.

Refactor attribute validation into validate and validatebasic
2 participants