Flet / eslint-to-esformatter

ISC License
30 stars 3 forks source link

Things to keep/discard #2

Closed Nate-Wilkins closed 9 years ago

Nate-Wilkins commented 9 years ago

Pretty much go through some of the things I wrote in esformatter-transformer-eslintrc

Rule Transformer

Here's the current interface

module.exports = {
    // indicate that this rule is supposed to be empty (ie no transform)
    // could change this to check if we have a transform or not
    informative: false,

    // used for needed esformatter plugins for eslintrc configurations to work properly
    dependencies: [], // or function (ruleOptions)

    // rule options to esformatter options manipulation
    transform: function (ruleOptions, esformatterOptions) {
         // instead of returning 'new' esformatter options I end up using `object-path`
         // so I don't have to do things like:
         if (esformatterOptions.lineBreak && esformatterOptions.lineBreak && esformatterOptions.lineBreak.after) {
         esformatterOptions.lineBreak.after.ArrayExpression = 1;
        }
    }
};

I do like the object-path module because it's easy to use and technically we shouldn't ever get conflicts with the esformatter rules. Not sure about the dependencies implementation but I don't know what else would work well.

As a side note I would like to include all the rules eslint supports even if they don't transform (including the deprecated ones?) see #2

Module

The one thing that I noticed between out two frameworks was the inclusion of the first eslint argument for each rule. Was there any explicit reason behind this?

In mine I just skip over the transformation and it's dependencies if it's not activated. (>0) If it is active however I only pass the arguments that it uses not the activation argument.

Esformatter Plugin

As @millermedeiros suggested on #2 where we somehow, either within the convert or a separate method, get the plugins/dependencies needed to fully support the provided .eslintrc configuration.

exports.setOptions(opts) {
    var eslintrc = loadEslintrc();
    var converted = eslintToEsformatter.convert(eslintrc);
    // this is not supported yet but you could be abstracted in a different way as well
    // see: https://github.com/millermedeiros/esformatter/issues/308
    exports.sub = converted.plugins.map(function(id) {
        return require(id);
    });
};

Testing

I literally have 1 test so I can't contribute much here atm. As for the testing framework should we use the same framework esformatter does? If not they all do pretty much the same thing

We could create a separate npm module for the plugin or just include it alongside the module that does the conversion.

Flet commented 9 years ago
  1. including all rules and setting module.exports.informative = true is cool. I'm fine with this.
  2. object-path sounds like the lib I needed but was unaware existed :+1: Lets use it for all.
  3. First eslint argument I found that esformatter may apply some defaults if no configuration is passed. The key-spacing transform is an example of this. If the rule is off and -1 is not passed then esformatter will fall back to a default which changes the code. This is also the reason why I think every transform will need to be run even if the rule is not active...
  4. Required esformatter plugins If a plugin is required, I just envisioned adding the plugin name to the config under the plugins key... Since the goal of this lib is to just output esformatter json, thats all that is needed. A separate esformatter plugin that leverages this lib would be a better place for this logic (maybe esformatter-transformer-eslintrc could be that plugin).
  5. Testing I have a slightly clever test template that will do the following:
    1. Feed a rule to the transform method.
    2. Ensure the appropriate esformatter json fragment is output.
    3. Ensure that esformatter formats code in the expected way.

Check out the key-spacing test for an example.

Other Things I'd like to keep the current method signature if possible. Specifically, the input args are the eslint rule arguments and the method returns a fragment of esformatter json configuration (which is merged to the main return object in index.js).

I just pushed this lib to npm as 1.0.0-alpha.1. We can just keep incrementing (1.0.0-alpha.2 etc) until we're ready for 1.0.0, which should be when most rules are at least partially implemented.

I hope you're OK with using standard for linting. :innocent: It should make merging PRs from new contriubtors a bit less of a hassle. There are sublime and vi plugins to make it easier to adopt as well. I could be convinced to use semistandard if semicolons are in your heart. :smile:

This is an OPEN Open Source Project.

Nate-Wilkins commented 9 years ago

I'm ok with semistandard :heart: Do you want to stick to some kind of commit standard? angular?

Do you see this module being just the esformatter core? I would rather have it be esformatter + whatever plugins needed to get a pure eslintrc config formatting.

Created #5 to track stuff.

Flet commented 9 years ago

A commit standard should not be required... I don't see the scope of this repo getting that crazy. We should keep a simple changelog though.

I see this module as a simple translator between an eslintrc JSON object and an esformatter JSON object and thats it. esformatter itself should not be part of the regular dependencies. Note that in tests we're invoking esformatter to ensure the config matches an expected output, but that is devDep only...

Nate-Wilkins commented 9 years ago

Sounds good! Closing.