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

Make profiles query user parameter optional #539

Merged
merged 26 commits into from
Jul 27, 2021

Conversation

dadamu
Copy link
Contributor

@dadamu dadamu commented Jul 6, 2021

Description

This PR is the implementation of #534.

Closes #534

Checklist

  • Targeted PR against correct branch.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit tests.
  • Wrote integration tests (simulation & CLI).
  • Updated the documentation.
  • Added an entry to the CHANGELOG.md file.
  • Re-reviewed Files changed in the Github PR explorer.

@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

Merging #539 (2b4a15b) into master (3d93d51) will increase coverage by 0.03%.
The diff coverage is 96.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #539      +/-   ##
==========================================
+ Coverage   82.81%   82.84%   +0.03%     
==========================================
  Files          91       91              
  Lines        5470     5480      +10     
==========================================
+ Hits         4530     4540      +10     
  Misses        737      737              
  Partials      203      203              
Impacted Files Coverage Δ
x/profiles/types/query_app_links.go 0.00% <0.00%> (ø)
x/profiles/client/cli/cli_app_links.go 35.35% <100.00%> (+2.02%) ⬆️
x/profiles/client/cli/cli_chain_links.go 88.37% <100.00%> (+0.42%) ⬆️
x/profiles/client/cli/cli_dtag_requests.go 85.26% <100.00%> (+0.48%) ⬆️
x/profiles/client/cli/cli_relationships.go 80.00% <100.00%> (+0.68%) ⬆️
x/profiles/keeper/grpc_query.go 75.63% <100.00%> (-0.60%) ⬇️

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 3d93d51...2b4a15b. Read the comment docs.

@dadamu dadamu marked this pull request as ready for review July 6, 2021 09:31
Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

Can you please add some tests to make sure that the different comination (0, 1 args) work properly?

@dadamu
Copy link
Contributor Author

dadamu commented Jul 8, 2021

Unit tests added.

@dadamu dadamu requested a review from RiccardoM July 8, 2021 06:20
Copy link
Contributor

@leobragaz leobragaz left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

I think all the QueryUserXXXXX should be renamed to QueryXXXXX now since the user is optional. This applied to both *Request and *Response Proto definitions

proto/desmos/profiles/v1beta1/query_relationships.proto Outdated Show resolved Hide resolved
proto/desmos/profiles/v1beta1/query_relationships.proto Outdated Show resolved Hide resolved
@dadamu
Copy link
Contributor Author

dadamu commented Jul 23, 2021

@RiccardoM I see, then, the prefix User in rpc methods of them should be removed as well.

@dadamu dadamu requested a review from RiccardoM July 23, 2021 07:42
proto/desmos/profiles/v1beta1/query.proto Outdated Show resolved Hide resolved
proto/desmos/profiles/v1beta1/query.proto Outdated Show resolved Hide resolved
proto/desmos/profiles/v1beta1/query.proto Outdated Show resolved Hide resolved
proto/desmos/profiles/v1beta1/query.proto Outdated Show resolved Hide resolved
x/profiles/client/cli/cli_app_links.go Outdated Show resolved Hide resolved
x/profiles/client/cli/cli_chain_links.go Outdated Show resolved Hide resolved
dadamu and others added 3 commits July 27, 2021 14:28
Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
dadamu and others added 6 commits July 27, 2021 14:51
Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@dadamu dadamu requested a review from RiccardoM July 27, 2021 07:03
@RiccardoM RiccardoM merged commit a098636 into master Jul 27, 2021
@RiccardoM RiccardoM deleted the paul/query-params-optional branch July 27, 2021 09:17
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.

Make profiles query user parameter optional
3 participants