Skip to content

chore: update json tag APY to apy#1126

Merged
RafilxTenfen merged 4 commits intomainfrom
rafilx/proto-lower-case
Jul 12, 2022
Merged

chore: update json tag APY to apy#1126
RafilxTenfen merged 4 commits intomainfrom
rafilx/proto-lower-case

Conversation

@RafilxTenfen
Copy link
Copy Markdown
Contributor

Description

  • To be compatible with the other structures and avoid casting to lowercase on rust side as well

closes: #XXXX


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
  • added appropriate labels to the PR
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • 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 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)

@RafilxTenfen RafilxTenfen requested a review from a team as a code owner July 12, 2022 16:22
@RafilxTenfen RafilxTenfen self-assigned this Jul 12, 2022
@RafilxTenfen RafilxTenfen requested a review from toteki July 12, 2022 16:24
@RafilxTenfen RafilxTenfen requested a review from a team as a code owner July 12, 2022 16:25
@RafilxTenfen RafilxTenfen changed the title chore: update APY to apy chore: update json tag APY to apy Jul 12, 2022
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #1126 (efa25de) into main (c1a31c0) will not change coverage.
The diff coverage is 39.86%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1126   +/-   ##
=======================================
  Coverage   43.82%   43.82%           
=======================================
  Files          65       65           
  Lines        8393     8393           
=======================================
  Hits         3678     3678           
  Misses       4472     4472           
  Partials      243      243           
Impacted Files Coverage Δ
x/leverage/client/cli/tx.go 0.00% <0.00%> (ø)
x/leverage/keeper/msg_server.go 0.84% <0.00%> (ø)
x/leverage/types/tx.go 0.00% <0.00%> (ø)
x/leverage/types/codec.go 41.93% <50.00%> (ø)
x/leverage/simulation/operations.go 92.66% <90.69%> (ø)
x/leverage/client/tests/tests.go 100.00% <100.00%> (ø)
x/leverage/keeper/keeper.go 43.92% <100.00%> (ø)
x/leverage/keeper/validate.go 51.61% <100.00%> (ø)

@toteki
Copy link
Copy Markdown
Contributor

toteki commented Jul 12, 2022

We might also be able to change the APY field in the proto struct itself to lowercase - that would also make it consistent with other query response structs. (Side effect would be possibly appearing as Apy on the go side).

On the other hand, this json tag has the lowest impact on anything else so might be better.

Anyone else have thoughts on the best approach?

(gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec",
(gogoproto.nullable) = false
(gogoproto.nullable) = false,
(gogoproto.jsontag) = "apy"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this a standard practice? I think APY looks more correct to me, even for JSON. But I don't have a strong opinion for this.

Copy link
Copy Markdown
Contributor Author

@RafilxTenfen RafilxTenfen Jul 12, 2022

Choose a reason for hiding this comment

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

The official standard for JSON is actually camelCase but we have everything as snack_case '-'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Twitter and Facebook API's use snake_case while Microsoft and Google use camelCase.

@RafilxTenfen
Copy link
Copy Markdown
Contributor Author

We might also be able to change the APY field in the proto struct itself to lowercase - that would also make it consistent with other query response structs. (Side effect would be possibly appearing as Apy on the go side).

On the other hand, this json tag has the lowest impact on anything else so might be better.

Anyone else have thoughts on the best approach?

Following this approach that @robert-zaremba shared probably it is to be called APY on the go side, but in my opinion, being the first property name as acronyms I vote for keeping Apy

@toteki
Copy link
Copy Markdown
Contributor

toteki commented Jul 12, 2022

I'm leaning toward what you currently have in the PR - only changing the json tag at the moment.

@RafilxTenfen RafilxTenfen merged commit 2015720 into main Jul 12, 2022
@RafilxTenfen RafilxTenfen deleted the rafilx/proto-lower-case branch July 12, 2022 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants