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

preprocessor config, fix for #212 #222

Merged
merged 9 commits into from
Apr 22, 2014
Merged

preprocessor config, fix for #212 #222

merged 9 commits into from
Apr 22, 2014

Conversation

limikael
Copy link
Contributor

Hi,

Here is a fix for #212.

As I wrote in the comments there, it is possible to set a preprocessor directive in the options. The preprocessor is a node module which will be called to allow modifications of the data before rendering. The module is searched for relative to the current working directory.

Example yuidoc.json:

{
    "options": {
        "preprocessor": "internal_doc_preprocessor.js"
    }
}

Then, the file internal_doc_preprocessor.js would look like the following. In this case it is a preprocessor which removes all classes and class items which has the @internal tag:

module.exports=function(data) {
    var keepClasses={};

    for (var c in data.classes) {
        if (!data.classes[c].hasOwnProperty("internal"))
            keepClasses[c]=data.classes[c];
    }

    data.classes=keepClasses;

    var keepItems=[];

    for (var i in data.classitems) {
        if (!data.classitems[i].hasOwnProperty("internal"))
            keepItems.push(data.classitems[i])
    }

    data.classitems=keepItems;
}

I also added a test suite which tests the functionality, with respect to single or multiple processors being used, correct path resolution in case the preprocessor use absolute or relative path, etc.

Please take a look!

@limikael
Copy link
Contributor Author

Uh... The tests are failing, but it is not in my code... They fail for node 0.8 but work for node 0.10. What to do?

@@ -43,7 +43,8 @@ YUI.add('yuidoc', function (Y) {
version: '0.1.0',
paths: [],
themedir: path.join(__dirname, 'themes', 'default'),
syntaxtype: 'js'
syntaxtype: 'js',
preprocessor: undefined
Copy link
Member

Choose a reason for hiding this comment

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

not need to define an undefined value.

@caridy
Copy link
Member

caridy commented Mar 19, 2014

@limikael I like what you have here, it just need some more work. Please, try to align with the style (formatting styles) of the rest of the program, it seems what you're code is using your own style :/, e.g.: if (foo===1) {, where we normally do if (foo === 1) { etc, etc, etc.

@limikael
Copy link
Contributor Author

@caridy Thanks for the feedback! Ok I will take a look and get back with some fixes. Yes you are probably right that I have been using my own style, will align with the rest of the project.

@limikael
Copy link
Contributor Author

@caridy Hi again! Sorry for the delay, but I have now cleaned up the code so it should be according to the code standards the rest of the project use.

With regards to where to search for the modules, I think it makes more sense actually to search for them relative to the pwd, since that is how other paths work, such as the options for 'outdir' and 'paths'.

@limikael
Copy link
Contributor Author

@caridy Hi again! Did you get a chance to take a second look at it? If you feel that it needs more work still please tell me and I will be happy to contribute that.

@limikael limikael mentioned this pull request Apr 16, 2014
preprocessors = [].concat(this.options.preprocessor);

preprocessors.forEach(function (preprocessor) {
var preprocessorModule = require(path.resolve(preprocessorsRelativeTo, preprocessor));
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also consider passing preprocessors in a form of nodejs modules, saying I have a nodejs module that export a function, and I want to install that at the top level as devDependencies, then passing that name into the preprocessor option. For that, I think we can do:

try {
   preprocessorModule = require(preprocessor);
} catch (e) {
   preprocessorModule = require(path.resolve(preprocessorsRelativeTo, preprocessor));
}

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 makes sense. This way it will be possible to create "yuidoc plugins" and distribute them as separate npm modules as well, so I will fix this...

@caridy
Copy link
Member

caridy commented Apr 16, 2014

@limikael added two recommendations, aside from that, it looks great.

…g relative to config file.

* return data when applying preprocessors rather than mutate the data in place.
* pass config options to preprocessors.
@limikael
Copy link
Contributor Author

@caridy Ok thanks again for the feedback! Please take a look now, I have made these changes:

  • I changed the function applyPreprocessors to runPreprocessors and it now acts on a clone of the data, and it returns the mutated clone. The cloning is done by data=JSON.parse(JSON.stringify(data)) which might look a bit hackish and slow, but my impression from a bit of googling is that it is actually quite fast compared to other cloning methods. Also, it is only done if there actually are some preprocessors to apply.
  • It now also tries to load the preprocessors as standard npm modules. I did this by using the method that you suggested and it seems to work. I also added a test case for this.

Also, "on my own initiative":

  • I made a small change to how the preprocessors are called, so that the config options is passed as a second argument. This makes sense in context of loading the preprocessors as npm modules, since it will then be possible to create configurable plugins.

And it seems like the travis build works now, including my 5 new testcaeses... 👍

@caridy caridy merged commit 64b2acc into yui:master Apr 22, 2014
caridy added a commit that referenced this pull request Apr 22, 2014
caridy added a commit that referenced this pull request Apr 22, 2014
@caridy
Copy link
Member

caridy commented Apr 22, 2014

  • yuidocjs@0.3.50

@caridy
Copy link
Member

caridy commented Apr 22, 2014

@limikael thanks for the commitment to get this done. Now we need to iterate, test this, and document it, also we can start creating new pkgs like yuidoc-preprocessor-<foo> or something like that. We can have a dedicate section in the README for preprocessors.

@limikael
Copy link
Contributor Author

@caridy Ok great! I have thought about writing a blog post with a tutorial and some example yuidoc-preprocessor-<foo> to be referenced from the post. Maybe parts of that blog post can be extracted and go into a README as well.

I will keep you posted of progress!

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.

2 participants