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: Porting io/fs package #1323

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

feat: Porting io/fs package #1323

wants to merge 13 commits into from

Conversation

notJoon
Copy link
Member

@notJoon notJoon commented Oct 31, 2023

Description

  • Porting major codes from io/fs package
  • Create testing/fstest

Note: sort.Slice is not supported yet (this method has internal/reflectlite as an dependency). So, I've temporarily filled in the blanks with a basic sorting algorithm - like Quick Sort - which will need to be fixed later.


related with: #1267

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Oct 31, 2023
@notJoon notJoon changed the title porting io/fs package feat: Porting io/fs package Oct 31, 2023
@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b1a53c0) 55.91% compared to head (204124c) 55.09%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1323      +/-   ##
==========================================
- Coverage   55.91%   55.09%   -0.82%     
==========================================
  Files         420      421       +1     
  Lines       65415    66441    +1026     
==========================================
+ Hits        36578    36608      +30     
- Misses      25977    26957     +980     
- Partials     2860     2876      +16     

see 32 files with indirect coverage changes

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

@notJoon
Copy link
Member Author

notJoon commented Nov 1, 2023

ReadDir method use the Sort package, but this package imports reflectile, which isn't implemented yet. So, I have to find a way to bypass it.

@notJoon
Copy link
Member Author

notJoon commented Nov 2, 2023

test failed

Test Failed on examples/gno.land/r/demo/boards/z_4_filetest.gno. I think it is related with time package.

@notJoon notJoon marked this pull request as ready for review November 6, 2023 09:36
@notJoon notJoon requested review from jaekwon, moul and a team as code owners November 6, 2023 09:36
Copy link

netlify bot commented Nov 13, 2023

Deploy Preview for gno-docs2 canceled.

Name Link
🔨 Latest commit 204124c
🔍 Latest deploy log https://app.netlify.com/sites/gno-docs2/deploys/6551e79fd39d0f0008c5a3de

@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Nov 13, 2023
gnovm/stdlibs/io/fs/readdir.gno Show resolved Hide resolved
Comment on lines +111 to +113
// func (d dirInfo) 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.

Why is this commented?

Copy link
Member

Choose a reason for hiding this comment

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

Keep the comments from the original, as well as the copyright header.

gnovm/stdlibs/io/fs/readdir.gno Show resolved Hide resolved
gnovm/stdlibs/testing/fstest/mapfs.gno Show resolved Hide resolved
gnovm/stdlibs/testing/fstest/mapfs.gno Show resolved Hide resolved
// if err := fstest.TestFS(myFS, "file/that/should/be/present"); err != nil {
// t.Fatal(err)
// }
// func TestFS(fsys fs.FS, expected ...string) error {
Copy link
Member

Choose a reason for hiding this comment

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

Why removed?


// checkDir checks the directory dir, which is expected to exist
// (it is either the root or was found in a directory listing with IsDir true).
// func (t *fsTester) checkDir(dir string) {
Copy link
Member

Choose a reason for hiding this comment

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

also why removed?

Copy link
Member

Choose a reason for hiding this comment

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

This one needs to have commented functions uncommented, or if they rely on features we don't support then they need an appropriate // XXX comment

as well as testfs_test.go, please :)

@notJoon
Copy link
Member Author

notJoon commented Dec 5, 2023

@thehowl First of all, thanks for the detailed review as always. It's been quite some time since I add this, and there are many parts I simply marked "no" and commented out. So, It's challenging to make immediate corrections. Thus, I plan to revert to a "draft" and make comprehensive changes :)

@notJoon notJoon marked this pull request as draft December 5, 2023 02:49
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 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

2 participants