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): System path library #1452

Merged
merged 13 commits into from
Dec 3, 2022
Merged

Conversation

alex-snezhko
Copy link
Member

A path module for the standard library, which should close #1384. For the most part the API and implementation of this module is based on https://github.com/reasonml/reason-native/tree/master/src/fp, with a few differences and some additional features added.

This module doesn't interact with the actual file system, rather just providing functions for manipulating the data representation of paths.

A summary of the API:

  • main type: Path<a>. Like ReasonNative's Fp, this type is parameterized in order to be able to strongly type relative/absolute paths.
  • Relative and Absolute are opaque types that can be used to parameterize a path
  • absolute(String) -> Option<Path<Absolute>>: parses an absolute path, returns None if invalid
  • relative(String) -> Option<Path<Relative>>: parses a relative path, returns None if invalid
  • append(Path<a>, Path<Relative>) -> Path<a>: append a relative path to a path
  • appendString(Path<a>, String) -> Option<Path<a>>: like append but parses the string path
  • relativeTo(Path<a>, Path<a>) -> Result<Path<Relative>, RelativizationError>: constructs a relative path (from 2 of the same path types) that would take one path to the other. Fails if either both are absolute paths but on different roots e.g. /bin and C:/Users or if there is not enough context to create a relative path from 2 relative paths e.g. getting from ../here to ./there
  • parent(Path<a>) -> Path<a>: returns the path of the parent directory of the path
  • name(Path<a>) -> Option<String>: returns the "file name" portion of the path if it exists
  • stem(Path<a>) -> Option<String>: returns the "file name" portion of the path without the extension if it exists
  • extension(Path<a>) -> Option<String>: returns the extension of the "file name" portion of the path if it exists
  • root(Path<Absolute>) -> AbsoluteRoot
  • toString(Path<a>, Encoding) -> String: stringifies the path with either POSIX (/) or Windows-style (\) path separators

This is somewhat of a proof-of-concept, so let me know if there are any differing opinions on how paths should be encoded or if anything about the API should change.

stdlib/path.gr Outdated Show resolved Hide resolved
@alex-snezhko alex-snezhko marked this pull request as ready for review November 24, 2022 01:36
Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

This is looking sweet! My big request is to add a fromString function that parses any type of filepath string and returns the appropriate type. I believe this will need to return a PathWithType (but correct me if I'm wrong).

The most important aspects of this module are the entry/exit points (fromString and toString) so they should be the first functions you see in the functions section.

stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

I love these updates. I had some feedback.

compiler/test/stdlib/path.test.gr Outdated Show resolved Hide resolved
compiler/test/stdlib/path.test.gr Outdated Show resolved Hide resolved
compiler/test/stdlib/path.test.gr Show resolved Hide resolved
compiler/test/stdlib/path.test.gr Show resolved Hide resolved
compiler/test/stdlib/path.test.gr Outdated Show resolved Hide resolved
stdlib/path.gr Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

🔥 - just a couple of nits about the docs

stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Show resolved Hide resolved
@phated
Copy link
Member

phated commented Dec 1, 2022

Looks like there might be a non-exhaustive warning happening somewhere too.

@alex-snezhko
Copy link
Member Author

Ah yeah I'm assuming that's from the

let TBase(base) = base

This is an enum with a single constructor so it shouldn't ever not match but I'll change the implementation to get rid of the warning

@phated
Copy link
Member

phated commented Dec 2, 2022

@alex-snezhko seems to be something else because the warning still displays on your latest push

stdlib/path.gr Outdated
Comment on lines 244 to 245
[first, ...rest] => {
let tokens = [first, ...rest]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[first, ...rest] => {
let tokens = [first, ...rest]
[first, ...rest] as tokens => {

I believe we can use as syntax here. I'm also wondering why it doesn't warn about not handling [first]... I assume [first, ...rest] matches any non-empty list and just makes rest an empty list if there is exactly 1 item.

@phated
Copy link
Member

phated commented Dec 2, 2022

I merged in main and reformatted the test file to fix the failure.

stdlib/path.gr Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
stdlib/path.gr Outdated Show resolved Hide resolved
Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

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

This is really fantastic, @alex-snezhko. I know a tremendous amount of work went into this, and the result is stunning. And thanks @phated for all of the feedback along the way! Really beautiful, high-quality, idiomatic work.

stdlib/path.gr Outdated Show resolved Hide resolved
Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Heroic effort! 💪 Thanks for all the work here @alex-snezhko

@phated phated merged commit 900e976 into grain-lang:main Dec 3, 2022
@github-actions github-actions bot mentioned this pull request Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

stdlib: add path libary
3 participants