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

Fix: Exclude eol-last from output (fixes #48) #49

Merged
merged 1 commit into from
Apr 21, 2016
Merged

Fix: Exclude eol-last from output (fixes #48) #49

merged 1 commit into from
Apr 21, 2016

Conversation

btmills
Copy link
Member

@btmills btmills commented Apr 15, 2016

/cc @BYK for review

@@ -12,6 +12,9 @@ var assign = require("object-assign");
var remark = require("remark");

var SUPPORTED_SYNTAXES = ["js", "javascript", "node", "jsx"];
var UNSATISFIABLE_RULES = [
Copy link
Member

Choose a reason for hiding this comment

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

May wanna make this configurable at a future version.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. I don't believe we have a way to support plugin-specific configuration. Is that correct, @ilyavolodin? If other core rules comes up, we can add them to this list. Hopefully by the time we get requests to exclude rules in third-party plugins, we have glob configs that will allow users to exclude those rules on their own.

Copy link
Member

Choose a reason for hiding this comment

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

As in allow rules inside plugins to specify if they should apply to markdown or not? Or configure all of the rules inside plugin in a specific way? If former - then no.

Copy link
Member Author

Choose a reason for hiding this comment

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

A way to apply config to a postprocess step in a plugin. Something like:

{
    "pluginConfig": {
        "markdown": {
            "unsatisfiableRules": ["eol-last", "react/no-multi-comp"]
        }
    }
}

I checked on the plugin documentation and didn't see anything like that.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... You could use settings. Although I'm having trouble finding it in the documentation for some reason.
Ah, found it (yay, search!), it's strange that it's not mentioned in working with plugins, but it is mentioned in the configuring eslint: http://eslint.org/docs/user-guide/configuring#adding-shared-settings

Copy link
Member Author

Choose a reason for hiding this comment

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

settings only gets passed to rules though. If necessary, we could:

  1. Modify core to pass settings to processors.
  2. Hack it in by offering a markdown/unsatisfiable-rules meta-rule that would remember its config.
  3. Shave an entire herd of yacks and get glob config in core.

Copy link
Member

Choose a reason for hiding this comment

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

I vote for shaving yacks! You get nice sweaters that way too!

Copy link
Member

Choose a reason for hiding this comment

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

Let's do both? :D

@BYK
Copy link
Member

BYK commented Apr 16, 2016

LGTM

Only concern is a bout tests. It's not clear to me from the PR that this new feature is properly covered by tests.

@btmills
Copy link
Member Author

btmills commented Apr 18, 2016

@BYK Previously, all of the plugin tests would blow up if the exclusion wasn't working because I enabled eol-last in the integration tests. However, you make a good point that it wasn't clear that's what was happening, so I added an additional postprocess test to make it explicit what's supposed to happen. What do you think now?

@BYK
Copy link
Member

BYK commented Apr 19, 2016

@btmills - looks great, prefrect!

@btmills btmills merged commit 90f9ac0 into master Apr 21, 2016
@btmills btmills deleted the eol-last branch April 21, 2016 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants