Skip to content

Merge mkdir and mkdirRecursive #58

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

Merged
merged 3 commits into from
Mar 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ Notable changes to this project are documented in this file. The format is based
## [Unreleased]

Breaking changes:
- Update `mkdir` to take `Boolean` arg for `recursive` option (#53, #55, #58 by @JordanMartinez)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... This isn't entirely correct as it was mkdir' that was updated now. I'll need to submit a new PR.


New features:
- Add bindings to `mkdir(path, { recursive: true })` via `mkdirRecursive` (#53, #55 by @JordanMartinez)
- Update project and deps to PureScript v0.15.0 (#59 by @JordanMartinez, @thomashoneyman, @sigma-andex)

Bugfixes:
Expand Down
31 changes: 6 additions & 25 deletions src/Node/FS/Async.purs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ module Node.FS.Async
, unlink
, rmdir
, mkdir
, mkdirRecursive
, mkdir'
, readdir
, utimes
Expand Down Expand Up @@ -201,34 +200,16 @@ rmdir file cb = mkEffect $ \_ -> runFn2
mkdir :: FilePath
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're already going to break this, it may be better to provide an options record:

mkdir :: { dirname :: Filepath, recursive :: Boolean, perms :: Perms } -> Callback Unit -> Effect Unit

to avoid boolean blindness. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure we need this. mkdir is pretty well-known from things like bash and those using JavaScript are already familiar with mkdir { recursive: true }. I'm not sure what else the boolean it could be.

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed on the 0.15 call, we should just match the explicit API provided via Node:

https://nodejs.org/api/fs.html#fsmkdirpath-options-callback

mkdir :: FilePath -> { recursive :: Boolean, mode :: Perms } -> Callback Unit -> Effect Unit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

The code was already using mkdir', so should we continue using that name to reduce breakage? Or name it the very verbose, mkdirWithOptions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm personally fine with the primed option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Can I get an approval then?

-> Callback Unit
-> Effect Unit
mkdir path = mkdir' path { recursive: false, mode: mkPerms all all all }

mkdir path = mkdir' path (mkPerms all all all)

-- | Makes a new directory and any directories that don't exist
-- | in the path. Similar to `mkdir -p`.
mkdirRecursive :: FilePath
-> Callback Unit
-> Effect Unit

mkdirRecursive path = mkdirRecursive' path (mkPerms all all all)

-- | Makes a new directory (and any directories that don't exist
-- | in the path) with the specified permissions.
mkdirRecursive'
-- | Makes a new directory with the specified permissions.
mkdir'
:: FilePath
-> Perms
-> { recursive :: Boolean, mode :: Perms }
-> Callback Unit
-> Effect Unit
mkdirRecursive' file perms cb = mkEffect $ \_ -> runFn3
mkdirImpl file { recursive: true, mode: permsToString perms } (handleCallback cb)

-- | Makes a new directory with the specified permissions.
mkdir' :: FilePath
-> Perms
-> Callback Unit
-> Effect Unit
mkdir' file perms cb = mkEffect $ \_ -> runFn3
mkdirImpl file { recursive: false, mode: permsToString perms } (handleCallback cb)
mkdir' file { recursive, mode: perms } cb = mkEffect $ \_ -> runFn3
mkdirImpl file { recursive, mode: permsToString perms } (handleCallback cb)

-- | Reads the contents of a directory.
readdir :: FilePath
Expand Down
31 changes: 7 additions & 24 deletions src/Node/FS/Sync.purs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ module Node.FS.Sync
, rmdir
, mkdir
, mkdir'
, mkdirRecursive
, mkdirRecursive'
, readdir
, utimes
, readFile
Expand Down Expand Up @@ -172,33 +170,18 @@ rmdir :: FilePath
rmdir file = mkEffect $ \_ -> runFn1
rmdirSyncImpl file

-- | Makes a new directory.
mkdirRecursive
:: FilePath
-> Effect Unit
mkdirRecursive = flip mkdirRecursive' $ mkPerms all all all

-- | Makes a new directory with the specified permissions.
mkdirRecursive'
:: FilePath
-> Perms
-> Effect Unit
mkdirRecursive' file perms = mkEffect $ \_ -> runFn2
mkdirSyncImpl file { recursive: true, mode: permsToString perms }

-- | Makes a new directory.
mkdir :: FilePath
-> Effect Unit

mkdir = flip mkdir' $ mkPerms all all all
mkdir path = mkdir' path { recursive: false, mode: mkPerms all all all }

-- | Makes a new directory with the specified permissions.
mkdir' :: FilePath
-> Perms
-> Effect Unit

mkdir' file perms = mkEffect $ \_ -> runFn2
mkdirSyncImpl file { recursive: false, mode: permsToString perms }
mkdir'
:: FilePath
-> { recursive :: Boolean, mode :: Perms }
-> Effect Unit
mkdir' file { recursive, mode: perms } = mkEffect $ \_ -> runFn2
mkdirSyncImpl file { recursive, mode: permsToString perms }

-- | Reads the contents of a directory.
readdir :: FilePath
Expand Down