Skip to content
This repository has been archived by the owner on Mar 23, 2024. It is now read-only.

Better configuration, plugin support #731

Closed
wants to merge 11 commits into from
Closed

Better configuration, plugin support #731

wants to merge 11 commits into from

Conversation

mdevils
Copy link
Member

@mdevils mdevils commented Oct 30, 2014

What I did for this pull-request:

  • Remade the configuration: now it looks simpler, cleaner, straight.
  • Achieved 100% test coverage for the configuration.
  • Prepared sample changes for pluginization: Pluginization jscs-jsdoc#14
  • Created plugin support.
  • Wrote tests for plugin support.
  • Simplified CLI logic a bit.

@mikesherov, @markelog, @mrjoelkemp, please take a look.

(I will squash the changes before the merge)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.57%) when pulling bbe3ed3 on mdevils.plugins into 101eccb on master.

this._ruleSettings = processedSettings;
};

Configuration.prototype.getProcessedConfig = function() {
Copy link
Member

Choose a reason for hiding this comment

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

Docs are missing

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot it, thanks! Will fix today!

@mrjoelkemp
Copy link
Member

This is amazing work @mdevils. Bravo on getting this done and fully tested.

I have no glaring problems with this approach; I'm sure issues will arise in the near future we start working with (and extending) this new format.

👍

@mdevils
Copy link
Member Author

mdevils commented Oct 31, 2014

Thank you for your opinion and comments, Joel!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.58%) when pulling 004a797 on mdevils.plugins into 101eccb on master.

@mdevils
Copy link
Member Author

mdevils commented Nov 1, 2014

Fixed all the issues. Check it out again, please.

@qfox
Copy link
Member

qfox commented Nov 1, 2014

Unfortunately, nothing about getOptionName and rule mapping in plugins.

Am I right about registering plugin rules in global rule space? Is it legal? Is it right? If it is then I there would be conflicts. But also there could be extensions of basic rules probably... What we decide about it?

upd: I though we are in progress... But feels like I've missed something.

@mdevils
Copy link
Member Author

mdevils commented Nov 1, 2014

@zxqfox, the problem you are talking about does not exist yet. Implementing any solution for this problem now will result in much higher complexity, which I would not like to have. In future it will not be a breaking change to introduce namespacing. But I believe, we should do this ONLY if we realize the problem exists and it is serious enough.

@qfox
Copy link
Member

qfox commented Nov 1, 2014

@mdevils I've stuck in loading something what I've called subrules because of subtree of jsdoc in jscs-jsdoc plugin. So it's already exists for me :-) Now I thinking how to do it easier and about plugins that could want to have some subrules routing as well as jsdoc plugin have. This problem is just about subrules and some common preparation for a bunch or subrules so probably it's a really very private problem and it won't break the most part of potential plugins that just use esprima ast tree.

So the main problem just in data preparation before calling rules itself. Atm I see 2 solutions.

  1. So jsdoc comments will be parsed before running main bunch of rules declared in config;
  2. Or just share some initial code between rules to call it each time and cache the result.

If we going (1) we should change jscs itself to call some triggers (or something) before calling rules that registered by plugin. While for (2) we don't need to change anything in jscs but we should make local router in each plugin and it will force jscs to call "empty" rules.

So for (1) plugin developer would register each rules independently and also probably some triggers for some events (to force parsing jsdoc once after esprima did some with file). While for (2) there are no difference in registering rules because we should do it manually in plugins.

For sure I believe it should be done in jscs for D.R.Y. and some optimizations (to call only what user want). At least would be great if some suboption logic will be shared with plugins (to not repeat this code in each plugin). But actually, if consumer uses this plugin by declaring it in config then he knows what he doing ... Well I hope you see now what I'm talking about.

@mdevils
Copy link
Member Author

mdevils commented Nov 2, 2014

@zxqfox, hard to get the idea. Can you put examples to describe it? Configuration examples?

@qfox
Copy link
Member

qfox commented Nov 3, 2014

@mdevils I've had some ideas

In plugin we can do something like registering helpers for a bunch of rules (or in global space):

config.registerFileHelper('jsdoc-comments-parser');
config.registerRule('jsdoc-some-rule');
config.registerRule('jsdoc-another-rule');

And then in rules we've got already parsed jsdoc comments in file object. Or some special methods for parsing and caching them in file object instance.

Or maybe we can just extend basic classes with some additional helpers to find scopes (closest function in es-ast), blocks (closest block scope for let, const), etc.

So atm I can't parse file comments once (or maybe can with ast tree fixing, or caching in helper), iterate once, and register multiple different rules that using parsed comments.

It's not a big trouble because we can live without it, but it would be great if there will be a simple mechanism in jscs to provide some hooks for plugins. Atm it's registering rules, configuring rules, and checking files. Probably we can expose something else? To be clear, my need is onFileLoad or onBeforeCheck hook where I could parse comments (or prepare file, etc.) and extend ast nodes with jsdoc property for all rules checking jsdoc.

@mrjoelkemp
Copy link
Member

Can we move the discussion of the plugin system to another issue? I'd like to land this config change since it's holding up other PRs.

@qfox
Copy link
Member

qfox commented Nov 3, 2014

@mrjoelkemp Yeah, sure. Sorry 🍭

@mdevils
Copy link
Member Author

mdevils commented Nov 3, 2014

@zxqfox please create an issue so we can keep our discussion going.

@mdevils
Copy link
Member Author

mdevils commented Nov 3, 2014

Merged this.

@mdevils mdevils closed this Nov 3, 2014
@mdevils mdevils deleted the mdevils.plugins branch November 3, 2014 18:11
@markelog
Copy link
Member

markelog commented Nov 5, 2014

@mdevils could create a repo with plugin example?

@qfox
Copy link
Member

qfox commented Nov 5, 2014

@markelog
Copy link
Member

markelog commented Nov 5, 2014

@zxqfox i saw it, but i was asking about new plugin interface that introduced in this pull.

Like how you should install it and configure it.

@qfox
Copy link
Member

qfox commented Nov 5, 2014

@markelog I can do that if you want

@markelog
Copy link
Member

markelog commented Nov 5, 2014

That would be helpful

@qfox
Copy link
Member

qfox commented Nov 10, 2014

  1. It doesn't work for relative paths at travis ;-( https://travis-ci.org/jscs-dev/jscs-jsdoc/jobs/40507024
  2. It does work as plugin with jscs#master at dev machine https://github.com/zxqfox/jscs-jsdoc/tree/feature/pluginization

@markelog
Copy link
Member

  1. Could you create an issue?

@qfox
Copy link
Member

qfox commented Nov 10, 2014

@markelog Yeah, sure. #759

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

Successfully merging this pull request may close these issues.

6 participants