refactor(mkdir): consistent error handling #854

nfischer · 2018-06-03T01:47:11Z

This refactors mkdir() to (for the most part) invoke mkdirSync() and
catch any exceptions, instead of the error-prone approach of trying to
predict when specific errors would happen.

This adds two new tests for "name too long" and that it's OK to mkdir -p a directory which already exists. The latter was implicitly covered
by another test case, but this should be explicit.

Lastly, this changes error messages to be more consistent with Linux
mkdir, and adds the "avoid-escape" rule in the test directory to allow
using double-quoted strings when it aids readability.

Fixes #722

This refactors mkdir() to (for the most part) invoke mkdirSync() and catch any exceptions, instead of the error-prone approach of trying to predict when specific errors would happen. This adds two new tests for "name too long" and that it's OK to `mkdir -p` a directory which already exists. The latter was implicitly covered by another test case, but this should be explicit. Lastly, this changes error messages to be more consistent with Linux mkdir, and adds the "avoid-escape" rule in the test directory to allow using double-quoted strings when it aids readability. Fixes #722

nfischer · 2018-06-03T01:47:50Z

@freitagbr take a look, but this should probably be considered as a breaking change because it changes error messages.

nfischer · 2018-06-03T01:49:22Z

src/mkdir.js

@@ -51,29 +51,13 @@ function mkdirSyncRecursive(dir) {
 function _mkdir(options, dirs) {
  if (!dirs) common.error('no paths given');

-  if (typeof dirs === 'string') {


I think the type is always string. We introduced some code to flatten out arrays. Even in the case of ShellStrings, I think we also convert those into bare strings.

nfischer · 2018-06-03T01:50:41Z

src/mkdir.js

-      var reason;
-      if (e.code === 'EACCES') {
-        reason = 'Permission denied';
-      } else if (e.code === 'ENOTDIR' || e.code === 'ENOENT') {


I think ENOENT should be handled distinctly from ENOTDIR, not sure why they're in the same branch here.

Hmmm this is weird. On unix, ENNOTDIR and ENOENT are for different reasons. On appveyor, I see ENOENT when part of the path is a regular file. Not sure yet how we should handle this.

Is this still causing the appveyor CI to fail?

Yes. I haven't fixed this yet.

nfischer · 2018-06-03T01:51:21Z

src/mkdir.js

+        EEXIST: 'File exists',
+        ENAMETOOLONG: 'File name too long',
+        ENOENT: 'No such file or directory',
+        ENOTDIR: 'Not a directory',


I limited this to the list of error codes I could repro locally. I think that's fine for now, and we can add more later.

Is this enum something that should be declared globally? Perhaps in src/common.js?

Some of the error codes can be shared (e.g., EEXIST). However, there's no real advantage to moving these to common.js. It's the difference between common.codes.EEXIST and 'EEXIST'.

When it comes to user-facing error messages, I think each command wants a different message as it sees fit.

Ok, makes sense. This enum could at least be declared globally in this file.

nfischer · 2018-06-03T01:51:59Z

test/.eslintrc.json

@@ -11,6 +11,7 @@
    "no-param-reassign": "off",
    "no-console": "off",
    "curly": "off",
+    "quotes": ["error", "single", "avoid-escape"],


Let me know if you'd like to promote this to the top-level config.

Are there any escaped quotes outside of test/? I can't find any in src/.

Guess there's not. Let's keep it here then.

nfischer · 2018-06-03T01:53:10Z

test/mkdir.js

@@ -145,6 +159,21 @@ test('-p flag', t => {
  shell.rm('-Rf', `${t.context.tmp}/a`); // revert
 });

+test('create same dir twice with -p flag', t => {


This is implicitly covered in the "globbed dir" test, but this merits explicit coverage.

This didn't fix the issue for Windows. This reverts commit 03d08f3.

freitagbr · 2018-06-21T03:28:44Z

src/mkdir.js

+        EEXIST: 'File exists',
+        ENAMETOOLONG: 'File name too long',
+        ENOENT: 'No such file or directory',
+        ENOTDIR: 'Not a directory',


Ok, makes sense. This enum could at least be declared globally in this file.

nfischer added refactor bash compat Compatibility issues with bash or POSIX behavior breaking Breaking change labels Jun 3, 2018

nfischer requested a review from freitagbr June 3, 2018 01:47

move dir into temp dir

17b10b9

nfischer commented Jun 3, 2018

View reviewed changes

nfischer added 2 commits June 2, 2018 19:08

try to join paths differently in test

03d08f3

Revert "try to join paths differently in test"

c28319f

This didn't fix the issue for Windows. This reverts commit 03d08f3.

freitagbr requested changes Jun 21, 2018

View reviewed changes

move object to module scope

95bfdd1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(mkdir): consistent error handling #854

refactor(mkdir): consistent error handling #854

nfischer commented Jun 3, 2018

nfischer commented Jun 3, 2018

nfischer Jun 3, 2018

nfischer Jun 3, 2018

nfischer Jun 3, 2018

freitagbr Jun 27, 2018

nfischer Jun 29, 2018

nfischer Jun 3, 2018

freitagbr Jun 20, 2018

nfischer Jun 20, 2018

freitagbr Jun 21, 2018

nfischer Jun 26, 2018

nfischer Jun 3, 2018

freitagbr Jun 20, 2018

nfischer Jun 20, 2018

nfischer Jun 3, 2018

freitagbr Jun 21, 2018

refactor(mkdir): consistent error handling #854

Are you sure you want to change the base?

refactor(mkdir): consistent error handling #854

Conversation

nfischer commented Jun 3, 2018

nfischer commented Jun 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment