-
Notifications
You must be signed in to change notification settings - Fork 48
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
Discussion implementation #48
Conversation
Signed-off-by: RiccardoM <[email protected]>
Codecov Report
@@ Coverage Diff @@
## develop #48 +/- ##
==========================================
- Coverage 81.47% 80.8% -0.67%
==========================================
Files 13 14 +1
Lines 502 547 +45
==========================================
+ Hits 409 442 +33
- Misses 84 98 +14
+ Partials 9 7 -2
Continue to review full report at Codecov.
|
x/posts/internal/types/post.go
Outdated
} | ||
|
||
func NewPost(ID, parentID PostID, message string, created int64, owner sdk.AccAddress) Post { | ||
func NewPost(id, parentID PostID, message string, allowsComments bool, created int64, owner sdk.AccAddress) Post { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we better follow the rule. https://github.com/golang/go/wiki/CodeReviewComments#initialisms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, by running the make lint
command, if we change id
to ID
, the following error will be shown up:
x/posts/internal/types/post.go:77:14: captLocal: `ID' should not be capitalized (gocritic)
func NewPost(ID, parentID PostID, message string, allowsComments bool, created int64, owner sdk.AccAddress) Post {
^
I think we should following coding rules that can be automatically checked from GolangCI (which we should enable for this repo as the config file already exists)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is lacking of something. My original idea of having this kind of interaction is to cater some discussions on a specific purpose. The usual Post
> Reply
> Reply
> Reply
is already a discussion. The first use case I thought of was how to make this mechanism work on Cosmos Hub. For example, a discussion on the governance proposal. We have a lot of discussions on Telegram, just a few discussions on Cosmos forum. But if I'm a delegator which I'm not very always in the community, I will get lost in the Telegram discussion.
The solution to this is to attach a discussion forum directly on to the gov proposal page. I originally wanted to add a Riot chat on Big Dipper but found that delegators don't use Riot. Mintscan has done this (https://www.mintscan.io/proposals/19) but as you can see there is no discussion at all in any of the proposal as it's a Disqus where the users need to login with their centralized social login.
So my idea is to reference a Post
to the governance proposal page on Big Dipper. Then Atom holders can discuss on the same page as the gov proposal with their delegator address. They don't have to show their names as only validators will be referenced with their monikers (we should build our decentralized social profile). When they keep replying the top level post of about this gov proposal, it's a discussion. The initial Post
can only be submitted by the gov proposal proposer but this checking should be done on the frontend.
Let me know if this make sense and worth implementing. I do want to do this experiment though.
…/discussion-implementation � Conflicts: � x/posts/internal/types/post.go
Signed-off-by: RiccardoM <[email protected]>
… field Signed-off-by: RiccardoM <[email protected]>
Signed-off-by: RiccardoM <[email protected]>
I've added the |
Signed-off-by: RiccardoM <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Looking forward to see some demos.
Description
This PR implements the support for discussion as discussed into the proper issue.
Closes #13
Note. Please merge this after #42 and #44 have been merged.
Checklist
Files changed
in the Github PR explorer