-
-
Notifications
You must be signed in to change notification settings - Fork 113
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 user-friendly file system module #1966
base: main
Are you sure you want to change the base?
Conversation
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.
The code looks good to me, Most of my comments were related to graindoc.
Two other things:
- Currently the
sys/
path is used for more low levelwasi
interactions. I think we should either move these wasi interactions elsewhere or put this module elsewhere. - Are we currently able to add some better streaming functions such as
File.open
,File.readLine
andFile.close
?
be13437
to
c66b46b
Compare
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.
Overall, I love the API design! My major piece of feedback here is that I don't think this module belongs in the sys
subdirectory, but in the regular stdlib
folder. The sys
folder was really just meant for the low-level wasi stuff (and probably should have been named just wasi
).
I envision that moving forward to Preview 2, the wasi
folder will just have the generated bindings to each of the wasi standards, and we'll have high-level libraries in the regular standard library. We can note in the module doc that use of that library incurs a dependency on a particular wasi world. For now, we can say that use of the module requires WASI Preview 1.
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 read all the way through and didn't spot anything, looks like a nice usable interface too.
Thoughts on maybe in 0.6 we rename the |
We did make that rename happen. I do see a world in which it makes sense to still have this separated, though in environments where you're not allowed a filesystem or otherwise, WASI makes it pretty easy to virtualize. |
@alex-snezhko Do you mind rebasing this? |
@ospencer rebased |
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.
Everything looks great, I've been using this library for months locally and haven't run into a single problem.
Should we provide a Utf8.readLine
? in cases with big files having a streamed api seems helpful, maybe this is a future addition when we have scoped resouces so the user doesn't need to worry about File.open
and File.close
.
I was thinking we could build out a more comprehensive streaming API in the future, I feel like there are a few different directions we could go |
stdlib/fs.gr
Outdated
* | ||
* @since v0.6.5 | ||
*/ | ||
provide let makeSymlink = (linkContents, targetBaseDirPath=None, targetPath) => { |
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 this should be called createSymlink
.
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.
Ok going to rename makeDir
to createDir
as well for consistency
ac1fd34
to
600b4ad
Compare
Add a file system module that is more high-level and user-friendly than the current
sys/file
Closes #211