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

Allow custom test functions #81

Merged
merged 3 commits into from Jul 26, 2016

Conversation

alexlafroscia
Copy link
Contributor

@alexlafroscia alexlafroscia commented Jul 24, 2016

This feature is useful if using this plugin with a package like ember-mocha, which provides its own set of describe functions. A custom array can be provided so that those extensions can also be protected from having only called.

One thought about my changes is that it introduces a configuration object rather than an array. We could make the option parameter just an array, but I kind of like the extra context provided by the named keys. The difference is basically between this (how I have it right now)

{
     "rules": {
         "mocha/no-exclusive-tests": ["error", {
             "additionalTestFunctions": [
                 "describeModule"
             ]
         }]
     }
 }

and this (which I think provides less context and it more confusing)

{
     "rules": {
         "mocha/no-exclusive-tests": ["error", [
             "describeModule"
         ]]
     }
 }

Closes #80

@coveralls
Copy link

coveralls commented Jul 24, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 1026819 on alexlafroscia:allow-custom-test-functions into 6594359 on lo1tuma:master.

This feature is useful if using this plugin with a package like
`ember-mocha`, which provides its own set of `describe` functions.
A custom array can be provided so that those extensions can also be
protected from having `only` called.
@coveralls
Copy link

coveralls commented Jul 24, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 8c28af6 on alexlafroscia:allow-custom-test-functions into 6594359 on lo1tuma:master.

@coveralls
Copy link

coveralls commented Jul 24, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 8c28af6 on alexlafroscia:allow-custom-test-functions into 6594359 on lo1tuma:master.

@coveralls
Copy link

coveralls commented Jul 24, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling a2cbf40 on alexlafroscia:allow-custom-test-functions into 6594359 on lo1tuma:master.

@alexlafroscia
Copy link
Contributor Author

Sorry about the messy Coveralls comments, I overwrote my commits a few times.

@lo1tuma
Copy link
Owner

lo1tuma commented Jul 25, 2016

Thanks for the Pull Request.

It seems like adding those options to each rule introduces a lot of duplication, especially if we add more rules to this plugin in the future.
What do you think about using shared settings (e.g. as eslint-plugin-import or eslint-plugin-backbone does)?
With shared settings the configuration would look like this:

{
     "rules": {
         "mocha/no-exclusive-tests": "error",
         "mocha/no-skipped-tests": "error"
     },
    "settings": {
        "mocha/additionalTestFunctions": [ "describeModule" ],
        "mocha/additionalXFunctions": [ "xcustom" ]
    }
 }

or

{
     "rules": {
         "mocha/no-exclusive-tests": "error",
         "mocha/no-skipped-tests": "error"
     },
    "settings": {
        "mocha": {
            "additionalTestFunctions": [ "describeModule" ],
            "additionalXFunctions": [ "xcustom" ]
        }
    }
 }

@alexlafroscia
Copy link
Contributor Author

Huh, that's really cool. I didn't know you could configure ESLint that way -- I like it much better. Totally agree that the current approach means duplication, I'll fix up this PR to implement with shared settings instead.

@alexlafroscia
Copy link
Contributor Author

Added the shared config just like @lo1tuma suggested, allowing either the one-line or nested option object.

@lo1tuma
Copy link
Owner

lo1tuma commented Jul 26, 2016

Great work @alexlafroscia.

Thank you 🍻 .

@lo1tuma lo1tuma merged commit 7ac5780 into lo1tuma:master Jul 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants