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 path package #1065

Merged
merged 1 commit into from
Aug 28, 2023
Merged

Conversation

gfanton
Copy link
Member

@gfanton gfanton commented Aug 23, 2023

simply add path package to stdlib, needed by net/url package (and probably some other packages)

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.

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Aug 23, 2023
@gfanton gfanton mentioned this pull request Aug 23, 2023
9 tasks
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.

I've had a look through the Go native implementation. There doesn't seem to be any dependency on packages/features we can't currently support (imports are on errors, utf8 and internal/bytealg, all of which already exist as packages, and I don't see any channels/goros being used).

Could we have this as gno code instead of a native injection?

Also, gnovm/docs/go-gno-compatibility.md should be updated, too. :)

@gfanton
Copy link
Member Author

gfanton commented Aug 24, 2023

I've had a look through the Go native implementation. There doesn't seem to be any dependency on packages/features we can't currently support (imports are on errors, utf8 and internal/bytealg, all of which already exist as packages, and I don't see any channels/goros being used).
Could we have this as gno code instead of a native injection?

Sure, that make sense! I wasn't sure about preferring one location over another. I thought that if no modifications were needed, or if there were only a few methods to add, it would be more efficient to place this directly in the stdlib file rather than creating a specific gno package directory.

@thehowl
Copy link
Member

thehowl commented Aug 24, 2023

I thought that if no modifications were needed, or if there were only a few methods to add, it would be more efficient to place this directly in the stdlib file rather than creating a specific gno package directory.

In general, let's work towards keeping stdlibs.go injections as few as possible. They should be like C extensions in Python: they are possible and sometimes necessary, for either doing impossible/hard things in the VM language (Gno), or because of performance (and then again, isolate specific functions which need them).

Impossible/hard things include, for instance, any use of unsafe (ie. math.Float64frombits), or crypto (crypto/sha256), which is why those are injected.

strconv is currently native but I don't think all methods there are good candidates for native injections. They are still native in #859, but I'd like to change that in the future.

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
@thehowl thehowl merged commit debf7d9 into gnolang:master Aug 28, 2023
49 checks passed
Doozers pushed a commit to Doozers/gno that referenced this pull request Aug 31, 2023
simply add `path` package to stdlib, needed by `net/url` package (and
probably some other packages)


<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
- [ ] 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>
@moul moul added this to the 🚀 main.gno.land (required) milestone Sep 16, 2023
moul added a commit that referenced this pull request Sep 22, 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] #1076 
- [x] #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>
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

3 participants