-
Notifications
You must be signed in to change notification settings - Fork 364
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
chore: refactor p/demo/ui #902
Conversation
ad617bc
to
203e312
Compare
Signed-off-by: Manfred Touron <94029+moul@users.noreply.github.com>
203e312
to
b0320f4
Compare
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.
pre-approving; some nits.
} | ||
|
||
type Raw struct { | ||
Content string | ||
} | ||
|
||
func (r Raw) String() string { | ||
func (r Raw) String(_ DOM) string { |
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.
Nit:
func (r Raw) String(_ DOM) string { | |
func (r Raw) String(DOM) string { |
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 wasn't aware we can do this.
Anyway, I prefer the other method which emphasizes more on the explicit ignore, like what we do with anonymous imports.
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.
Any other opinions?
I'm okay to change my habit here, but not sure what's the best.
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've reviewed Effective Go, Google and Uber Style guide and there doesn't seem to be any reference to what to do here.
Personally I interpret not even using _
as being an even stronger indicator that the argument is not used, and that it is put there only to match an interface. But I'm fine with whatever we go with.
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'm also okay with your solution, so please, someone else? :)
func (text H1) String(_ DOM) string { return "# " + string(text) + "\n" } | ||
func (text H2) String(_ DOM) string { return "## " + string(text) + "\n" } | ||
func (text H3) String(_ DOM) string { return "### " + string(text) + "\n" } | ||
func (text H4) String(_ DOM) string { return "#### " + string(text) + "\n" } | ||
func (text H5) String(_ DOM) string { return "##### " + string(text) + "\n" } | ||
func (text H6) String(_ DOM) string { return "###### " + string(text) + "\n" } | ||
func (text Bold) String(_ DOM) string { return "**" + string(text) + "**" } | ||
func (text Italic) String(_ DOM) string { return "_" + string(text) + "_" } | ||
func (text Paragraph) String(_ DOM) string { return "\n" + string(text) + "\n" } | ||
func (_ HR) String(_ DOM) string { return "\n---\n" } |
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.
remove blank identifier?
func (text Paragraph) String(_ DOM) string { return "\n" + string(text) + "\n" } | ||
func (_ HR) String(_ DOM) string { return "\n---\n" } | ||
|
||
func (text Code) String(_ DOM) string { |
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.
same here
Signed-off-by: Manfred Touron <94029+moul@users.noreply.github.com>
Signed-off-by: Manfred Touron <94029+moul@users.noreply.github.com>
Let's merge the changes and have a discussion about anonymous variables later. Once we reach a decision on the official approach, we should update either the CONTRIBUTING.md guidelines or a linter accordingly. |
Refactor
p/demo/ui
: various fixes and API updates.Blocks #898
Continues #527
Related with #781
Addresses #903
Contributors Checklist
BREAKING CHANGE: xxx
message was included in the descriptionMaintainers Checklist
CONTRIBUTING.md
BREAKING CHANGE:
in the body)