Skip to content
This repository has been archived by the owner on Oct 19, 2021. It is now read-only.

Fixes #311 #106. Copied parakeety/coffeelint-prefer-english-operator #316

Merged
merged 4 commits into from
Aug 19, 2014

Conversation

AsaAyers
Copy link
Collaborator

@AsaAyers AsaAyers commented Aug 8, 2014

@parakeety does everything here look ok? One potential option would have been to make your repo a dependency, but I think it makes more sense for all core rules to live in this repo. Pulling in an external rule could also result in different builds of the same version of CoffeeLint having different versions of the same rule.

I verified that installing 3rd party rules can overwrite core rules. Once I started testing that I found that npm only has coffeelint-prefer-english-operator 0.0.3. Users of your rule will not get the latest version when CoffeeLint upgrades. Would you mind pushing a new version before I merge this? Also, do you want to change your message, maybe to include a shortened link to a page explaining that they can remove your rule from the config?

@bjmiller
Copy link

I have a similar rule, which also disallows !, in favor of not. Should that be included here, too?

@AsaAyers
Copy link
Collaborator Author

Yes, I should add that check here too. I think !!truthyOrFalsyValue might be an exception though.

@bjmiller
Copy link

Here's the full code of my rule. I had to include UNARY tokens to catch the single bang.

module.exports = class NoLogicalOperators
  rule:
    name:        'no_logical_operators'
    level:       'error'
    message:     'Use words instead of symbols'
    description: 'This rule enforces our style preference of word operators over symbols.'

  tokens: ['COMPARE', 'UNARY', 'LOGIC']
  lintToken: (token, tokenApi) ->
    # Compare the actual token with the lexed token.
    actual_token = tokenApi.lines[tokenApi.lineNumber][token[2].first_column..token[2].last_column]
    err = context:
      switch actual_token
        when '==' then 'Replace "==" with "is"'
        when '!=' then 'Replace "!=" with "isnt"'
        when '!'  then 'Replace "!" with "not"'
        when '||' then 'Replace "||" with "or"'
        when '&&' then 'Replace "&&" with "and"'
        else undefined
    return (err if err.context?)

@bjmiller
Copy link

For my purposes, a !!value isn't necessary. I can't think of a case where doing something with a ? or a ?= isn't a better choice.

@AsaAyers
Copy link
Collaborator Author

Thanks for mentioning this and posting your rule. I was going to import this as-is, but I hadn't paid attention to the fact that it's still a line linter when it can be converted to use the lexical linter. I think I'll merge bits of the two.

@parakeety
Copy link

@AsaAyers Sorry that it took a long time to get back to you. I just pushed the new version of my repo to npm. Everything looks good to me!

@AsaAyers
Copy link
Collaborator Author

I kept the name, tests, and modified description/message, but replaced the implementation with a modified version of @bjmiller 's code. Since it used lintToken it eliminated the token loop and checking the token types, as they now happen over in src/lexical_linter.coffee.

AsaAyers added a commit that referenced this pull request Aug 19, 2014
Fixes #311 #106. Copied parakeety/coffeelint-prefer-english-operator
@AsaAyers AsaAyers merged commit d028c4e into master Aug 19, 2014
@bjmiller
Copy link

Should there be a separate rule for forbidding the use of !!expression, then?

@AsaAyers
Copy link
Collaborator Author

I don't think code should be written until it will be used. if someone wants to make a case for why not not thing is preferred over !!thing they can open a PR that adds a parameter. no_implicit_parens has a strict parameter that is pretty similar.

@AsaAyers AsaAyers deleted the prefer_english_operator branch August 19, 2014 14:05
@bjmiller
Copy link

That's kind of my point. I want this rule to forbid !!expression.

The correct idiom isn't not not expression. I want my team to use expression? instead.

Give me a few days, and I'll try to submit a PR to add the parameter.

@AsaAyers
Copy link
Collaborator Author

expression? is not a direct replacement for !!expression. Reading back through I'm not sure why I thought this, but I thought you were more passive on whether or not to forbid !!.

I can get that option added before I make the 1.6 release. I'll have special handling so that !! warns to replace with ? instead of counting as two errors/warnings.

@AsaAyers
Copy link
Collaborator Author

A rule can also pass a custom level along with the context. Now prefer_english_operators has a doubleNotLevel that defaults to ignore.

beb4ca4

@bjmiller
Copy link

This looks great. Thank you!

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.

None yet

3 participants