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

feat(examples): Implement markdown package #2912

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

sunspirit99
Copy link
Contributor

@sunspirit99 sunspirit99 commented Oct 5, 2024

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 package

Screenshot from 2024-10-23 19-28-43

cc @moul

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@sunspirit99 sunspirit99 requested review from a team as code owners October 5, 2024 12:59
@sunspirit99 sunspirit99 requested review from jaekwon and thehowl and removed request for a team October 5, 2024 12:59
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Oct 5, 2024
Copy link

codecov bot commented Oct 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.76%. Comparing base (4f27a57) to head (be3a999).
Report is 2 commits behind head on master.

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     
Flag Coverage Δ
contribs/gnodev 60.54% <ø> (ø)
contribs/gnofaucet 15.77% <ø> (+0.94%) ⬆️
gno.land 73.62% <ø> (ø)
gnovm 67.92% <ø> (ø)
misc/genstd 79.72% <ø> (ø)
tm2 62.39% <ø> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@moul
Copy link
Member

moul commented Oct 8, 2024

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 markdown to md.

}

// TodoList returns a list of todo items with checkboxes for markdown
func TodoList(items []TodoItem) string {
Copy link
Member

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
	{},
	{},
	// ...
})

Copy link
Contributor Author

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 ?

Copy link
Contributor Author

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 {
Copy link
Member

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.

Copy link
Contributor Author

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)
}
Copy link
Member

Choose a reason for hiding this comment

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

missing func Paragraph, maybe?

Copy link
Contributor Author

@sunspirit99 sunspirit99 Oct 8, 2024

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
Copy link
Member

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.

Copy link
Contributor Author

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) {
Copy link
Member

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?

Copy link
Member

@moul moul Oct 8, 2024

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.

Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

@moul moul left a 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.

@sunspirit99
Copy link
Contributor Author

sunspirit99 commented Oct 8, 2024

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

@leohhhn leohhhn changed the title feat(gno.land): Implement markdown package feat(examples): Implement markdown package Oct 10, 2024
@gfanton
Copy link
Member

gfanton commented Oct 14, 2024

@moul I'm fine with merging this, but I'm hesitant about using the md module for now. Multiple ideas are emerging for an md framework. Should we consider moving this to a submodule instead, allowing others to submit their ideas? I'm concerned about having multiple libraries without coherence, all doing the same thing.

Comment on lines 156 to 158
func EscapeMarkdown(text string) string {
return ufmt.Sprintf("``%s``", text)
}
Copy link
Contributor

@n0izn0iz n0izn0iz Oct 31, 2024

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 ``"

?

Copy link
Contributor Author

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

@gfanton gfanton mentioned this pull request Nov 5, 2024
6 tasks
@leohhhn
Copy link
Contributor

leohhhn commented Nov 7, 2024

@sunspirit99

Can you move this code to your personal/company namespace?

@sunspirit99
Copy link
Contributor Author

@sunspirit99

Can you move this code to your personal/company namespace?

752ebf3 i've moved to my new house

Copy link
Contributor

@leohhhn leohhhn left a 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.

@leohhhn leohhhn added the don't merge Please don't merge this functionality temporarily label Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
don't merge Please don't merge this functionality temporarily 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: In Progress
Status: In Review
Development

Successfully merging this pull request may close these issues.

5 participants