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

Better ban rule failure messages & allows for additional custom messages #1385

Merged
merged 4 commits into from
Jul 27, 2016
Merged

Better ban rule failure messages & allows for additional custom messages #1385

merged 4 commits into from
Jul 27, 2016

Conversation

ChrisMBarr
Copy link
Contributor

@ChrisMBarr ChrisMBarr commented Jul 8, 2016

This PR does 2 things.


1️⃣ Better Wording

The wording of the failure message for the ban rule (and the no-console rule since it is based on the ban rule) has been updated.

👎 Before

function invocation disallowed: object.someFunction

(The word "invocation" always reminded me of a wizard casting a spell... 🔮 )

👍 After

Calls to 'object.someFunction' are not allowed.


2️⃣ Optional Explanation Messages

Most TSLint rules are basically either turned on or off and they offer explanation text or it's generally understood why the rule is enabled in a project. One thing that's always bothered my and my teammates is when a function is banned it basically just says "this is banned" and it doesn't tell you why. Since I'm the one who turned on the linting rules I eventually get someone asking me "why can't I use _.filter? I can use in in this other project just fine..."

Well, now an optional 3rd parameter for each banned function call can be added. This text could offer an explanation as to why it's banned and/or offer an alternative that could be used instead of the banned call. Here's an example of how it would be configured

"ban": [
      true, 
      ["myApp", "someFunc", "'myApp.someFunc' has been deprecated, use 'myApp.otherFunc' instead"],
      ["_", "filter", "Use the native JavaScript 'myArray.filter' instead."],
      ["_", "forEach"] //no additional custom message here
]

And here's what the failure message would look like with the additional text appended 👏

Calls to '_.filter' are not allowed. Use the native JavaScript 'myArray.filter' instead.

],
},
},
optionExamples: [`[true, ["console", "log"], ["someObject", "someFunction"]]`],
optionExamples: [`[true, ["someObject", "someFunction"], ["someObject", "someFunction", "Optional explanation"]]`],
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want both banned functions to be someObject and someFunction, can we change those terms for one of them?

@jkillian
Copy link
Contributor

Looking good @ChrisMBarr! Two tiny changes above to make if you can, and can you merge master into this branch so it's mergeable?

@jkillian
Copy link
Contributor

Looking good, can you merge master here? I can't merge this PR as there are merge conflicts right now

@jkillian jkillian merged commit a049830 into palantir:master Jul 27, 2016
@ChrisMBarr ChrisMBarr deleted the custom-ban-messages branch July 27, 2016 15:24
soniro pushed a commit to soniro/tslint that referenced this pull request Aug 31, 2016
…ges (palantir#1385)

* Updating ban rule to have better messages, and an optional explanation message as a 3rd parameter
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.

3 participants