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

Eager ignore #372

Merged
merged 1 commit into from
Jan 13, 2016
Merged

Eager ignore #372

merged 1 commit into from
Jan 13, 2016

Conversation

mixonic
Copy link
Contributor

@mixonic mixonic commented Aug 8, 2015

Paths may be ignored because they contain a large number of files. The getDirs method should not actually descend into those ignored directories. Specifically in Ember.js our tmp/ and node_modules/ directories may both be extremely large and have complex symlinking that trolls the validatePaths method.

Additionally this commit flattens the recursion in getDirs.

@mixonic
Copy link
Contributor Author

mixonic commented Aug 8, 2015

Could use some guidance about what may be annoying travis. The error message is broken (yui/yuitest#29) unfortunately. Test pass locally (or the relevant ones do, there are a number of intermittent errors that were present on master).

mixonic added a commit to mixonic/ember-cli-yuidoc that referenced this pull request Aug 8, 2015
YUIDoc can ignore paths when building a project. 0.8.1 the implementation is not very efficient, but after yui/yuidoc#372 lands you should see this change significantly improve docs build times (and possibly fix builds- I cannot seem to finish building ember docs without it).
@okuryu
Copy link
Member

okuryu commented Aug 10, 2015

My guess that yui/yuitest#29 doesn't resolves failing build of this PR, because it seems to show different errors with patching yui/yuitest#29.
https://gist.github.com/okuryu/05aee6bfa5ff1b07b424

@mixonic
Copy link
Contributor Author

mixonic commented Aug 13, 2015

@okuryu these failures seem very unrelated though?

@mixonic
Copy link
Contributor Author

mixonic commented Sep 7, 2015

@okuryu looking at yuitest failures again, but they do not seem related here. I sense a disturbance in the npm versioning of a dependency.

This commit has a massive, huge impact on Ember and other codebases. Would appreciate any aid in getting it though, despite the slow progress of these codebases.

@okuryu
Copy link
Member

okuryu commented Sep 8, 2015

@mixonic Sorry for my delay! Please let me try to investigate this ASAP.

@dschmidt
Copy link

Any news? This change reduces our doc build times from about a minute to ~2secs, so I would really like to see this upstreamed.

@stefanpenner
Copy link
Collaborator

Test pass locally (or the relevant ones do, there are a number of intermittent errors that were present on master).

@mixonic A rebased version of this PR, consistently demonstrates this issue.

@stefanpenner
Copy link
Collaborator

@mixonic not quite sure yet why their is a failure, but it does appear your test case passes without your provided implementation. Which suggests the test isn't quite correct.

@mixonic
Copy link
Contributor Author

mixonic commented Dec 30, 2015

It seems setUp fails silently and doesn't stop the a suite from running :trollface: :trollface:

Some tests blindly run on the whole input dir, and since the test I added relies on a fixture that causes a failure those suites attempting to use all of input/ fail.

@stefanpenner
Copy link
Collaborator

@mixonic you likely should create a unit test, targeting Y.getDirs directly.

something like:

        var dirs = Y.getDirs(__dirname + '/input/with-symlink', {
          ignorePaths: [
            'a/b',
            'a/d',
            'c'
          ]
        });

            // paths: 'a',
            // ignorePaths: [
            //   'a/b',
            //   'a/d',
            //   'c'
            // ]
        Assert.isArray(dirs);
        console.log(dirs);
        Assert.areSame(1, dirs.length, 'Failed to retrieve all path options');
        Assert.areSame(dirs[0], 'expected Path');

@stefanpenner
Copy link
Collaborator

@mixonic once this is green (and reasonable) i'll make sure it gets merged.

@mixonic mixonic force-pushed the eager-ignore branch 4 times, most recently from de16e9c to a33cd43 Compare December 30, 2015 21:27
Assert.areNotSame(-1, dirs.indexOf(pathPrefix + '/one'), 'contains path /one');
Assert.areNotSame(-1, dirs.indexOf(pathPrefix + '/one/two'), 'contains path /one/two');
},
'test: ignores paths': function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test fails without the changes to lib/utils.js.

@mixonic
Copy link
Contributor Author

mixonic commented Dec 30, 2015

@stefanpenner The changes here are intended to more efficiently build a list of directories by avoiding traversal of ignored paths. There should be no changed behavior.

However I've added a test for getDirs that shows it now looks for an array of ignored paths, and have confirmed that fails as expected (and the tests confirming old behavior pass) on master.

var json = (new Y.YUIDoc({
quiet: true,
paths: ['input/'],
paths: [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests were naively presuming all fixtures were valid. I've narrowed the list to ones required for passing tests.


$ npm install -g istanbul ytestrunner yuitest

You are able to run unit tests by executing `npm test`.
Copy link
Member

Choose a reason for hiding this comment

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

Why did you need to do this? I think npm test should works after run npm install.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test command presumes that instanbul is installed as a global executable:

https://github.com/yui/yuidoc/blob/master/package.json#L90

Copy link
Collaborator

Choose a reason for hiding this comment

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

npm run * prefixes ./node_modules/.bin/ on the path, which should include istanbul as it is devDep https://github.com/yui/yuidoc/blob/master/package.json#L83

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okie dokie, I'll attempt to remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

donezo.

Paths may be ignored because they contain a large number of files. The
getDirs method should not actually descend into those ignored
directories.

Additionally this commit flattens the recursion in getDirs.
okuryu added a commit that referenced this pull request Jan 13, 2016
@okuryu okuryu merged commit d68d714 into yui:master Jan 13, 2016
@okuryu
Copy link
Member

okuryu commented Jan 13, 2016

Merged. I'll publish a new version to npm in this weekend.

@mixonic mixonic deleted the eager-ignore branch January 13, 2016 15:14
@okuryu
Copy link
Member

okuryu commented Jan 18, 2016

Published v0.10.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants