Processing of sub directories are not performed correctly with Windows #95

Merged
merged 1 commit into from Sep 13, 2013

Conversation

Projects
None yet
2 participants
@konitter
Contributor

konitter commented Sep 6, 2013

In order to use a back slash to separate directory on Windows, fixed cli.js.

@ghost ghost assigned marrs Sep 6, 2013

@marrs

This comment has been minimized.

Show comment
Hide comment
@marrs

marrs Sep 6, 2013

Collaborator

Thanks for contributing. Can you explain in more details what problem this is solving and how? I'm a little unclear from the summary and from the diff.

Also the code currently removes the filename from the array whereas your patch now keeps the filename. Is this intentional?

Collaborator

marrs commented Sep 6, 2013

Thanks for contributing. Can you explain in more details what problem this is solving and how? I'm a little unclear from the summary and from the diff.

Also the code currently removes the filename from the array whereas your patch now keeps the filename. Is this intentional?

@konitter

This comment has been minimized.

Show comment
Hide comment
@konitter

konitter Sep 10, 2013

Contributor

For example, I will source file has are as follows.

project-directory/
    css/
        foo/
            a.css
            b.css
        bar/
            c.css
            d.css

If normal operation, the menu should look like this.

a

However, the link is not in the drop-down menu like this image on Windows.

a

I try to output of this code of cli.js in the "var parts".

// Make `link` objects for the menu.
var menuLinks = function(files, basePath) {
  return files.map(function(file) {
    var parts = path.relative(basePath, file).split('/');console.log(parts);
    parts.pop(); // Remove filename
    ...
  })
  ...
};

The output will be like this.Please read the documentation of "path.relative()" for details.

[ 'bar\\c.css' ]
[ 'bar\\d.css' ]
[ 'foo\\a.css' ]
[ 'foo\\b.css' ]

Then, I try to output the code that I have changed.

// Make `link` objects for the menu.
var menuLinks = function(files, basePath) {
  return files.map(function(file) {
    var parts = path.dirname(file).split('/');console.log(parts);
    parts.shift(); // Remove base directory name
    ...
  })
  ...
};

The output will be like this.Please read the documentation of "path.dirname()" for details.

[ 'css', 'bar' ]
[ 'css', 'bar' ]
[ 'css', 'foo' ]
[ 'css', 'foo' ]

And, you can if you remove directory name of the source file to "css" using parts.shift();, to get the sub-directory name does not depend in the OS.

Contributor

konitter commented Sep 10, 2013

For example, I will source file has are as follows.

project-directory/
    css/
        foo/
            a.css
            b.css
        bar/
            c.css
            d.css

If normal operation, the menu should look like this.

a

However, the link is not in the drop-down menu like this image on Windows.

a

I try to output of this code of cli.js in the "var parts".

// Make `link` objects for the menu.
var menuLinks = function(files, basePath) {
  return files.map(function(file) {
    var parts = path.relative(basePath, file).split('/');console.log(parts);
    parts.pop(); // Remove filename
    ...
  })
  ...
};

The output will be like this.Please read the documentation of "path.relative()" for details.

[ 'bar\\c.css' ]
[ 'bar\\d.css' ]
[ 'foo\\a.css' ]
[ 'foo\\b.css' ]

Then, I try to output the code that I have changed.

// Make `link` objects for the menu.
var menuLinks = function(files, basePath) {
  return files.map(function(file) {
    var parts = path.dirname(file).split('/');console.log(parts);
    parts.shift(); // Remove base directory name
    ...
  })
  ...
};

The output will be like this.Please read the documentation of "path.dirname()" for details.

[ 'css', 'bar' ]
[ 'css', 'bar' ]
[ 'css', 'foo' ]
[ 'css', 'foo' ]

And, you can if you remove directory name of the source file to "css" using parts.shift();, to get the sub-directory name does not depend in the OS.

@marrs

This comment has been minimized.

Show comment
Hide comment
@marrs

marrs Sep 12, 2013

Collaborator

Thanks for this. Can you provide a unit test to go with the code change before I merge?

Collaborator

marrs commented Sep 12, 2013

Thanks for this. Can you provide a unit test to go with the code change before I merge?

@konitter

This comment has been minimized.

Show comment
Hide comment
@konitter

konitter Sep 13, 2013

Contributor

I'm sorry, but I do not know details about the unit test. Could you tell me what to do specifically?

It may be different from the one you are looking for, but the result of make test is as follows.

Before making changes to the code :

cli
✔ HTML filename
✖ Link objects from paths

AssertionError:
{ './':
   [ { name: 'a', href: 'a.html', directory: './' },
     { name: 'b', href: 'b.html', directory: './' },
     { name: 'd', href: 'c-d.html', directory: './' } ] }
deepEqual
{ './':
   [ { name: 'a', href: 'a.html', directory: './' },
     { name: 'b', href: 'b.html', directory: './' } ],
  c: [ { name: 'd', href: 'c-d.html', directory: 'c' } ] }
    at Object.deepEqual (c:\dev\styledocco-master\node_modules\nodeunit\lib\types.js:83:39)
    at Object.exports.Link objects from paths (c:\dev\styledocco-master\test\cli.js:15:8)
    at Object.<anonymous> (c:\dev\styledocco-master\node_modules\nodeunit\lib\core.js:235:16)
    at c:\dev\styledocco-master\node_modules\nodeunit\lib\core.js:235:16
    at Object.exports.runTest (c:\dev\styledocco-master\node_modules\nodeunit\lib\core.js:69:9)
    at c:\dev\styledocco-master\node_modules\nodeunit\lib\core.js:117:25
    at c:\dev\styledocco-master\node_modules\nodeunit\deps\async.js:508:13
    at iterate (c:\dev\styledocco-master\node_modules\nodeunit\deps\async.js:118:13)
    at c:\dev\styledocco-master\node_modules\nodeunit\deps\async.js:129:25
    at c:\dev\styledocco-master\node_modules\nodeunit\deps\async.js:510:17


parser
✔ Documentation and code blocks
✔ Sections

After changing the code :

cli
✔ HTML filename
✔ Link objects from paths

parser
✔ Documentation and code blocks
✔ Sections
Contributor

konitter commented Sep 13, 2013

I'm sorry, but I do not know details about the unit test. Could you tell me what to do specifically?

It may be different from the one you are looking for, but the result of make test is as follows.

Before making changes to the code :

cli
✔ HTML filename
✖ Link objects from paths

AssertionError:
{ './':
   [ { name: 'a', href: 'a.html', directory: './' },
     { name: 'b', href: 'b.html', directory: './' },
     { name: 'd', href: 'c-d.html', directory: './' } ] }
deepEqual
{ './':
   [ { name: 'a', href: 'a.html', directory: './' },
     { name: 'b', href: 'b.html', directory: './' } ],
  c: [ { name: 'd', href: 'c-d.html', directory: 'c' } ] }
    at Object.deepEqual (c:\dev\styledocco-master\node_modules\nodeunit\lib\types.js:83:39)
    at Object.exports.Link objects from paths (c:\dev\styledocco-master\test\cli.js:15:8)
    at Object.<anonymous> (c:\dev\styledocco-master\node_modules\nodeunit\lib\core.js:235:16)
    at c:\dev\styledocco-master\node_modules\nodeunit\lib\core.js:235:16
    at Object.exports.runTest (c:\dev\styledocco-master\node_modules\nodeunit\lib\core.js:69:9)
    at c:\dev\styledocco-master\node_modules\nodeunit\lib\core.js:117:25
    at c:\dev\styledocco-master\node_modules\nodeunit\deps\async.js:508:13
    at iterate (c:\dev\styledocco-master\node_modules\nodeunit\deps\async.js:118:13)
    at c:\dev\styledocco-master\node_modules\nodeunit\deps\async.js:129:25
    at c:\dev\styledocco-master\node_modules\nodeunit\deps\async.js:510:17


parser
✔ Documentation and code blocks
✔ Sections

After changing the code :

cli
✔ HTML filename
✔ Link objects from paths

parser
✔ Documentation and code blocks
✔ Sections

marrs added a commit that referenced this pull request Sep 13, 2013

Merge pull request #95 from konitter/master
Processing of sub directories are not performed correctly with Windows

@marrs marrs merged commit 2492fd5 into jacobrask:master Sep 13, 2013

1 check passed

default The Travis CI build passed
Details
@marrs

This comment has been minimized.

Show comment
Hide comment
@marrs

marrs Sep 13, 2013

Collaborator

Ah, so in fact we already had a test documenting this behaviour and (as would be expected) it was failing in Windows. In that case there's nothing more to do. Pull request merged with thanks. 👍

If you're interested, the unit test for this case can be found in test/cli.js

Collaborator

marrs commented Sep 13, 2013

Ah, so in fact we already had a test documenting this behaviour and (as would be expected) it was failing in Windows. In that case there's nothing more to do. Pull request merged with thanks. 👍

If you're interested, the unit test for this case can be found in test/cli.js

@konitter

This comment has been minimized.

Show comment
Hide comment
@konitter

konitter Sep 13, 2013

Contributor

You're welcome 👍

Contributor

konitter commented Sep 13, 2013

You're welcome 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment