-
Notifications
You must be signed in to change notification settings - Fork 375
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
feat(examples): Implement markdown package #2912
base: master
Are you sure you want to change the base?
feat(examples): Implement markdown package #2912
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2912 +/- ##
==========================================
- Coverage 63.77% 63.76% -0.02%
==========================================
Files 548 548
Lines 78681 78681
==========================================
- Hits 50180 50170 -10
- Misses 25117 25130 +13
+ Partials 3384 3381 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
It's heading in the right direction by avoiding HTML, unlike other recent PRs. However, the current implementation isn't complete or appealing enough for me yet, though it may be for others. I believe it makes sense to view this PR not as a UI framework proposal but as a minimal md library. This library can be used by you or anyone else to build various UI frameworks. I'm fine with that direction. Therefore, you should rename |
} | ||
|
||
// TodoList returns a list of todo items with checkboxes for markdown | ||
func TodoList(items []TodoItem) 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.
not sure if it would be better, but it could be nice to find a solution that would avoid having to use custom types and avoid something like this:
tl := markdown.TodoList([]markdown.TodoItem{ // this line is the one I dislike
{},
{},
// ...
})
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.
0aaf911 do u like this change ?
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.
changed
func TodoList(items []TodoItem) string
to
func TodoList(items []string, done []bool) string
} | ||
|
||
// CodeBlock returns a code block for markdown, optionally specifying the language | ||
func CodeBlock(code string, language string) 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.
Maybe we could create individual helpers for CodeBlock
and LanguageCodeBlock
. We don't need to adhere strictly to markdown specifications. Instead, we should aim for a good compromise regarding helper names and parameters that will facilitate effective documentation generation and autocompletion.
The current approach might be the best, but a more usage-based method could also be worth considering.
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.
// Footnote returns a footnote for markdown | ||
func Footnote(reference, text string) string { | ||
return ufmt.Sprintf("[%s]: %s", reference, text) | ||
} |
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.
missing func Paragraph
, maybe?
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.
@@ -0,0 +1,118 @@ | |||
package markdown |
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.
This package is a perfect example of something that would benefit from a realm-based demo r/demo/md
. The demo should iterate over the available helpers, displaying their usage and the corresponding results.
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.
renamed to md
} | ||
|
||
// NewTable creates a new Table instance, ensuring the header and rows match in size | ||
func NewTable(header []string, rows [][]string) (*Table, error) { |
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.
What is the advantage of having a NewTable that returns a Stringer? Wouldn't it be enough to just have a func Table
?
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.
It would be convenient to have this helper, or perhaps a second one that accepts Stringer
or interface{}
instead of string
. Alternatively, we could keep this as a standalone package.
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 personally enjoy this usage, which is compatible with ufmt
: https://pkg.go.dev/text/tabwriter
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.
not sure i understood your idea correctly, plz take a look at the last commit changes
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 provided some opinionated feedback. I believe the current library is similar to ufmt
and can be merged early, allowing us to improve it over time.
I would prefer if you renamed it to md
. Additionally, please create a demo realm to showcase its capabilities.
That said, I'm fine with merging early.
@moul thanks for reviewing, i've made some changes, hope these things make sense to go ahead. I also raise another PR to apply this package into current demo realms soon |
@moul I'm fine with merging this, but I'm hesitant about using the |
examples/gno.land/p/demo/md/md.gno
Outdated
func EscapeMarkdown(text string) string { | ||
return ufmt.Sprintf("``%s``", text) | ||
} |
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.
can't this be bypassed by doing something like
text := "`` some-malicious-md ``"
?
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.
Thanks for the catch, i tested the example with potential Markdown syntax, and it successfully retains the original text without rendering Markdown. The only effect is the removal of the outer backticks
Can you move this code to your personal/company namespace? |
752ebf3 i've moved to my new house |
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.
Let's merge and see how people use this lib; maybe it will need some iteration.
cc @moul
EDIT: awaiting some internal discussions; I'll remove the "don't merge" label after them.
From #2753
I keep this PR open to see if I am on the right approach. If it's suitable, I'll investigate to make further improvements and provide implementation examples in the
Render()
functions of some current demo realms to demonstrate the use of this packagecc @moul
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description