Merge mkdir and mkdirRecursive #58
Conversation
JordanMartinez
added
purs-0.15
| @@ -199,36 +198,19 @@ rmdir file cb = mkEffect $ \_ -> runFn2 | |||
|
|
|||
| -- | Makes a new directory. | |||
| mkdir :: FilePath | |||
There was a problem hiding this comment.
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 Unitto avoid boolean blindness. What do you think?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 UnitThere was a problem hiding this comment.
Done!
The code was already using mkdir', so should we continue using that name to reduce breakage? Or name it the very verbose, mkdirWithOptions?
There was a problem hiding this comment.
I'm personally fine with the primed option.
There was a problem hiding this comment.
Ok. Can I get an approval then?
| @@ -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) | |||
There was a problem hiding this comment.
Hm... This isn't entirely correct as it was mkdir' that was updated now. I'll need to submit a new PR.
Description of the change
Fixes #54
Checklist: