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

Post and Like structure revist #40

Merged
merged 5 commits into from
Nov 19, 2019
Merged

Post and Like structure revist #40

merged 5 commits into from
Nov 19, 2019

Conversation

RiccardoM
Copy link
Contributor

@RiccardoM RiccardoM commented Nov 15, 2019

Description

This PR implements various Post and Like structure improvements.
Also, the various messages have been revisited in order to delete useless time fields.
Tests have been added and documentation has been written.

Closes #39

Checklist

  • Targeted PR against correct branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Re-reviewed Files changed in the Github PR explorer

- Removed the Namespace and ExternalOwner fields
- Changed how the likes for a given post are stored
Fixed some tests and formatted them using the gotests style
@RiccardoM RiccardoM added kind/enhancement Enhance an already existing feature; no "New feature" to add x/posts Post module labels Nov 15, 2019
@RiccardoM RiccardoM added this to the v0.1.0 milestone Nov 15, 2019
@kwunyeung
Copy link
Contributor

Let's target merging it this weekend and launch testnet.

Signed-off-by: RiccardoM <riccardo.montagnin@gmail.com>
@codecov-io
Copy link

codecov-io commented Nov 18, 2019

Codecov Report

Merging #40 into develop will increase coverage by 20.29%.
The diff coverage is 74.57%.

Impacted file tree graph

@@             Coverage Diff              @@
##           develop      #40       +/-   ##
============================================
+ Coverage    61.18%   81.47%   +20.29%     
============================================
  Files           13       13               
  Lines          541      502       -39     
============================================
+ Hits           331      409       +78     
+ Misses         192       84      -108     
+ Partials        18        9        -9
Impacted Files Coverage Δ
x/posts/internal/keeper/querier.go 0% <ø> (ø) ⬆️
x/magpie/internal/keeper/handler.go 87.93% <0%> (-3.15%) ⬇️
x/posts/internal/types/post.go 4% <0%> (-1.89%) ⬇️
x/posts/internal/types/msgs.go 100% <100%> (+56.16%) ⬆️
x/posts/internal/keeper/keeper.go 100% <100%> (+10.46%) ⬆️
x/magpie/internal/keeper/querier.go 88.88% <100%> (+22.22%) ⬆️
x/magpie/internal/keeper/keeper.go 100% <100%> (+20.45%) ⬆️
x/magpie/internal/types/session.go 88.23% <100%> (+88.23%) ⬆️
x/magpie/internal/types/msgs.go 96% <100%> (+61.21%) ⬆️
x/posts/internal/keeper/handler.go 91.08% <84.61%> (-2.55%) ⬇️
... and 6 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 e0ee3a1...e387f2a. Read the comment docs.

if err := keeper.CreateSession(ctx, session); err != nil {
// Check for any previously existing session
if _, found := keeper.GetSession(ctx, session.SessionID); found {
return sdk.ErrUnknownRequest(fmt.Sprintf("Session with id %s already exists", session.SessionID)).Result()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal but looks strange if a function is named GetSession where the function is to check whether the session exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GetSession here is used to check if a session already exists, but elsewhere (i.e inside the querier) the same method is used to get a session using its ID.

Here we use the second returned value, which allows for faster and easier comparison than the first one. It's just a convinient way to keep the code DRY and avoiding having multiple methods with very similar functionality

if err := k.SaveLike(ctx, like); err != nil {
return err
// Check for double likes
if likes.ContainsOwnerLike(like.Owner) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible to query the state before broadcasting the message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unluckily, not. The ValidateBasic method has no access to the state.

On the other hand, we can move this to the handler. Do you think it would be better?

Copy link
Contributor

@kwunyeung kwunyeung 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. Just a few comments.

Signed-off-by: RiccardoM <riccardo.montagnin@gmail.com>
@RiccardoM RiccardoM mentioned this pull request Nov 19, 2019
@kwunyeung kwunyeung merged commit 6f7e035 into develop Nov 19, 2019
@RiccardoM RiccardoM deleted the riccardo/like-revisit branch November 20, 2019 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Enhance an already existing feature; no "New feature" to add x/posts Post module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid double liking
3 participants