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: add p/moul/txlink + p/moul/helplink #2887

Merged
merged 10 commits into from
Oct 25, 2024
Merged

feat: add p/moul/txlink + p/moul/helplink #2887

merged 10 commits into from
Oct 25, 2024

Conversation

moul
Copy link
Member

@moul moul commented Oct 2, 2024

This PR aimed to promote the use of a p/ library for managing special help links from contracts.

It also provided an opportunity for me to realize that our discussion about changing the $ symbol would require some parsing and detection from the gnoweb perspective. If we want a simple library like this one, the goal should be to ideally craft a link to the current package without specifying the realm path. Relative URLs worked well with ?, but they won't function with $.

As an alternative, we can have this package look for std.PrevRealm().PkgAddr if it is not specified.

cc @jeronimoalbi @thehowl @leohhhn

Related with #2602
Related with #2876

@moul moul self-assigned this Oct 2, 2024
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Oct 2, 2024
Signed-off-by: moul <[email protected]>
@moul moul marked this pull request as ready for review October 2, 2024 17:14
@moul moul requested review from a team as code owners October 2, 2024 17:14
@moul moul requested review from n2p5 and mvertes and removed request for a team October 2, 2024 17:14
Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.62%. Comparing base (603f6d3) to head (4824aa9).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2887      +/-   ##
==========================================
+ Coverage   63.38%   63.62%   +0.24%     
==========================================
  Files         566      566              
  Lines       79490    79656     +166     
==========================================
+ Hits        50388    50685     +297     
+ Misses      25710    25577     -133     
- Partials     3392     3394       +2     
Flag Coverage Δ
contribs/gnodev 60.57% <ø> (ø)
contribs/gnofaucet 14.82% <ø> (ø)
gno.land 67.37% <ø> (ø)
gnovm 67.87% <ø> (ø)
misc/genstd 79.72% <ø> (ø)

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.

@leohhhn
Copy link
Contributor

leohhhn commented Oct 19, 2024

For me, it's ease of use with this kind of library. I found it simpler to just write out the constant up top, put in placeholders for specific arguments, and just populate it with a Sprintf. Helplink still makes me do manual concatenation in some cases, and in situations where there is more than one argument Sprintf's % operator is more practical, imho.

These two code paragraphs have the same output:

	// using helplink
	out += "#### "
	out += hl.Func(strconv.Itoa(i.upvote.Size())+like, "Upvote", "pkgpath", i.pkgpath)
	out += " - "
	out += hl.Func(strconv.Itoa(i.downvote.Size())+like, "Downvote", "pkgpath", i.pkgpath)
	out += "\n\n"

	// using sprintf
	const likesBar = "#### [%d 👍](/r/demo/hof?help&__func=Upvote&pkgpath=%s) - [%d 👎](/r/demo/hof?help&__func=Downvote&pkgPath=%s)\n\n"
	out += ufmt.Sprintf(
		likesBar,
		i.upvote.Size(),
		i.pkgpath,
		i.downvote.Size(),
		i.pkgpath,
	)

The long package-level constant might be a little ugly, but I think Sprintf's practicality beats helplink.

I have to play around a bit more but for now I'm not fully convinced currently.

@moul
Copy link
Member Author

moul commented Oct 19, 2024

i think it’s because you’re using a dynamic text with number; maybe in your case you should use the FuncURL and not the Func helper.

@moul
Copy link
Member Author

moul commented Oct 24, 2024

TODO: rename helplink to txlink, and make it just about the URL of funcs.

Edit: kept the two packages, with distinct roles.

@moul moul changed the title feat: add p/moul/helplink feat: add p/moul/txlink + p/moul/helplink Oct 25, 2024
@moul moul changed the title feat: add p/moul/txlink + p/moul/helplink feat: add p/moul/txlink + p/moul/helplink Oct 25, 2024
Signed-off-by: moul <[email protected]>
@moul
Copy link
Member Author

moul commented Oct 25, 2024

Based on #2809, I decided to create these two new libraries in my personal realm, p/moul, instead of the usual p/demo. I also updated r/demo/boards to use txlink instead of creating links manually.

There is a question about whether we want to use personal namespaces starting today or if it still makes sense to find a better location.

Signed-off-by: moul <[email protected]>
@thehowl thehowl merged commit 49e718c into master Oct 25, 2024
117 of 118 checks passed
@thehowl thehowl deleted the dev/moul/helplink branch October 25, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Status: ✅ Done
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants