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(stdlib): add net/url #1066

Merged
merged 11 commits into from
Sep 22, 2023
Merged

feat(stdlib): add net/url #1066

merged 11 commits into from
Sep 22, 2023

Conversation

gfanton
Copy link
Member

@gfanton gfanton commented Aug 23, 2023

Added the net/url package. This package will be beneficial for manipulating URLs, for instance, parsing query parameters or for URL muxing in realms.

depends:

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.

@gfanton gfanton self-assigned this Aug 23, 2023
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Aug 23, 2023
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Like #1065, just needs an update to gnovm/docs/go-gno-compatibility.md. Aside from that, once #1065 is merged, I think we can merge this as well.

gnovm/stdlibs/net/url/url.gno Outdated Show resolved Hide resolved
gnovm/stdlibs/net/url/url_test.gno Show resolved Hide resolved
@thehowl
Copy link
Member

thehowl commented Aug 24, 2023

It looks like CI is failing too.

First issue is because we now have the "net" directory it is considered "existing" as a package. Possible solution: change tm2/pkg/sdk/vm/builtins.go PackageGetter from using osm.DirExists to also determining whether there are any .gno files in the dir?

Hopefully with this also the second issue should be solved (seems roughly of same nature).

@ilgooz
Copy link
Contributor

ilgooz commented Aug 24, 2023

Should we have a net dir at all? Is it possible for a package or contract to interact with the network, or be aware of it? If not, it might make sense to just drop net/ part from the package path.

@thehowl
Copy link
Member

thehowl commented Aug 24, 2023

Good point @ilgooz. I reviewed the net/ directory, and no subpackages look like good candidates to be included in Gno.

I think net/url has the key advantage of being go-"compatible". Ie., importing a go file which uses net/url doesn't mean changing import path. It's quality-of-life, but I think it's helpful.

I propose changing the import path to url but having net/url as an alias

@gfanton
Copy link
Member Author

gfanton commented Aug 24, 2023

First issue is because we now have the "net" directory it is considered "existing" as a package. Possible solution: change tm2/pkg/sdk/vm/builtins.go PackageGetter from using osm.DirExists to also determining whether there are any .gno files in the dir?

thanks for the tip, I will try to do that.

Should we have a net dir at all? Is it possible for a package or contract to interact with the network, or be aware of it? If not, it might make sense to just drop net/ part from the package path.

Wouldn't it be better to keep it in sync with the original stdlib structure to avoid confusion? Can't we explicitly mark those kinds of packages as unusable and warn the user upon import?

@gfanton gfanton requested a review from a team as a code owner August 25, 2023 12:11
@github-actions github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Aug 25, 2023
@gfanton
Copy link
Member Author

gfanton commented Aug 25, 2023

change tm2/pkg/sdk/vm/builtins.go PackageGetter from using osm.DirExists

I had to change imports.go PackageGetter to have an impact on those failing tests, I've managed to make it works (d5c2563) by updating the behavior of ReadMemPackage: skipping validatePkgName if no gno files are present, and then do the same things in TestStore so package net can fallback on native method if needed.

…no files)

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
add 'Cut', 'CutPrefix' and 'CutSuffix' methods to "strings" package

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
@github-actions github-actions bot removed 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Sep 5, 2023
@moul moul added this to the 🌟 main.gno.land (wanted) milestone Sep 8, 2023
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Looks good 💯

I've left just a small comment, other than that it seems good to go


import (
"bytes"
// encodingPkg "encoding"
Copy link
Member

Choose a reason for hiding this comment

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

Was this intentionally left commented out?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes ! since it's copy/past from the orignal golang files, I prefere to simply comment instead of removing the line to easily find & fix if we add this package one day !

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.05% 🎉

Comparison is base (cfefb3b) 46.97% compared to head (7bcac2d) 47.03%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1066      +/-   ##
==========================================
+ Coverage   46.97%   47.03%   +0.05%     
==========================================
  Files         365      365              
  Lines       61156    61156              
==========================================
+ Hits        28729    28763      +34     
+ Misses      30066    30037      -29     
+ Partials     2361     2356       -5     
Flag Coverage Δ
gno.land-_test.gnokey 0.00% <ø> (ø)
gno.land-_test.gnoland 88.14% <ø> (ø)
gno.land-_test.pkgs 27.88% <ø> (ø)
gnovm-_test.cmd 45.89% <ø> (ø)
gnovm-_test.gnolang.native 63.09% <ø> (ø)
gnovm-_test.gnolang.other 16.63% <ø> (ø)
gnovm-_test.gnolang.pkg0 17.98% <ø> (ø)
gnovm-_test.gnolang.pkg1 8.21% <ø> (ø)
gnovm-_test.gnolang.pkg2 9.87% <ø> (ø)
gnovm-_test.gnolang.realm 41.68% <ø> (ø)
gnovm-_test.gnolang.stdlibs 53.53% <ø> (ø)
gnovm-_test.pkg 25.96% <ø> (ø)
tm2-_test.flappy ∅ <ø> (∅)
tm2-_test.pkg.amino 58.32% <ø> (ø)
tm2-_test.pkg.bft 63.81% <ø> (+0.37%) ⬆️
tm2-_test.pkg.others 59.15% <ø> (-0.06%) ⬇️

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

see 7 files with indirect coverage changes

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

gfanton and others added 2 commits September 20, 2023 15:23
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
@moul moul merged commit 2ecbb6d into gnolang:master Sep 22, 2023
179 checks passed
gfanton added a commit to gfanton/gno that referenced this pull request Nov 9, 2023
Added the `net/url` package. This package will be beneficial for
manipulating URLs, for instance, parsing query parameters or for URL
muxing in realms.

depends:
- [x] gnolang#1076 
- [x] gnolang#1065 

<details><summary>Contributors' checklist...</summary>

- [X] 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
- [X] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [X] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants