-
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
feat(stdlibs): add package html
#2802
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2802 +/- ##
=======================================
Coverage 60.85% 60.85%
=======================================
Files 563 563
Lines 75193 75193
=======================================
+ Hits 45759 45761 +2
+ Misses 26070 26069 -1
+ Partials 3364 3363 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
2b7396b
to
ee01d0c
Compare
gnovm/stdlibs/strings/replace.gno
Outdated
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 suggest you do another PR which adds support for strings.Replacer, and then re-base this one on top of that.
- We should not introduce more "temporary" code in the standard library, when we are gearing up towards mainnet.
- This code has the fault of being "iterative" on the replacements; ie.
NewReplacer("a", "bb", "b", "cc").Replace("a")
returnscccc
rather thanbb
.
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.
You're right thanks for the review :)
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.
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 see other changes concern removing sync.Once.
LGTM after we fixup strings.Replacer in another PR.
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 will want to remove this, but it looks good to add it while we're still exploring. lgtm.
gnovm/stdlibs/html/entity.gno
Outdated
var entity2 map[string][2]rune | ||
|
||
// populateMapsOnce guards calling populateMaps. | ||
var populateMapsOnce bool |
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 we remove this bool and check by the map size?
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.
Or maybe you can populate these maps directly in the declaration? It is guaranteed to be initialized only once.
Why is populateMaps
necessary ?
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.
you're right; this is the kind of optimization that somewhat makes sense for go but i think it's fine if we discard for gno.
so let's just have these in the declarations directly.
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.
Sure thanks @petar-dambovaliev @thehowl for the review indeed it is not required 👍
I removed the function and just initialize directly both tables on a5f4a1b
html
related to #1267
Maybe we should optimize it ? I did not wanted to do something complex as this is not the main point of this pull request
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description