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

Support for ERROR & WARN per rule #1738

Merged
merged 52 commits into from Mar 1, 2017

Conversation

Projects
None yet
8 participants
@olore
Copy link
Contributor

olore commented Nov 17, 2016

PR checklist

  • Addresses an existing issue: #629 #345
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

What changes did you make?

Allow a "mode" for each rule in tslint.json. Anything other than "off" and the rule will run, a value of "warn" will exit with a return code of 0

  "rules": {
    "interface-name": {
      "severity": "off"
    },
    "max-line-length": {
      "severity": "error",
      "options": 140
    },
    "member-ordering": {
      "severity": "warn",
      "options": [
        "public-before-private",
        "static-before-instance",
        "variables-before-functions"
      ]
    }
}

Is there anything you'd like reviewers to focus on?

  • I have a few questions, please see them as comments within the diff

olore added some commits Nov 15, 2016

Support config object in tslint.json
  * For example:
    ```
    "rules": {
      "align": {
        "mode": "off",
        "options": ["parameters", "arguments"]
      }
    }
    ```
 * See it run: `npm run compile && node ./lib/tslint-cli.js src/ruleLoader.ts`
 * Support mode: "warn", update proseFormatter to deal with warnings
* Formatter#format now takes failures,warnings,fixes (was failures, f…
…ixes)

* Refactor formatters to reduce duplication
* Add isWarning() to test Rule classes - these seem to be compiled from somewhere? Couldn't find source ts.
* Started work on getting all tests to pass
@palantirtech

This comment has been minimized.

Copy link
Member

palantirtech commented Nov 17, 2016

Thanks for your interest in palantir/tslint, @olore! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@@ -12,6 +12,7 @@ var Rule = (function (_super) {
Rule.prototype.apply = function (sourceFile) {
return this.applyWithWalker(new NoFailWalker(sourceFile, this.getOptions()));
};
Rule.prototype.isWarning = function() { return false; };

This comment has been minimized.

@olore

olore Nov 17, 2016

Contributor

I couldn't figure out where these files were generated from, so I just modified by hand.

This comment has been minimized.

@nchen63

nchen63 Nov 18, 2016

Contributor

I don't think the .ts files were checked in

@@ -45,19 +43,11 @@ describe("Stylish Formatter", () => {

const maxPositionTuple = `${maxPositionObj.line + 1}:${maxPositionObj.character + 1}`;

const expectedResult = colors.enabled ?

This comment has been minimized.

@olore

olore Nov 17, 2016

Contributor

this test was never run with colors disabled, so I removed the ternary

@@ -29,6 +30,8 @@ export abstract class AbstractRule implements IRule {

if (Array.isArray(value) && value.length > 1) {
ruleArguments = value.slice(1);
} else if (value.options) {
ruleArguments = arrayify(value.options);

This comment has been minimized.

@olore

olore Nov 17, 2016

Contributor

Allow the tslint.json maintainer to no be forced to put a single value in an array

@IllusionMH
Copy link
Contributor

IllusionMH left a comment

Any reason to use separate array for warnings instead of extending RuleFailure interface with mode property? (or level more speaking, however differs from config field)

In any case formatters should be updated, and I think it is easier to add field check, than comparing 2 arrays if they want group errors by file.

BTW may be it make sens to use level instead of mode since it is more widely used in such context?

RuleFailure -> RuleViolation
Extend RuleViolation to include a RuleLevel (ERROR|WARN)
@olore

This comment has been minimized.

Copy link
Contributor

olore commented Nov 18, 2016

@IllusionMH That makes a lot of sense to me. In fact it's where I started, but because of the huge number of changes, I backed it out. What do you think about moving RuleFailure to RuleViolation? A RuleViolation will have a level that can either be WARNING or ERROR.

To me, it's a cleaner solution, I was just apprehensive about making so many changes (100+ files).

@IllusionMH

This comment has been minimized.

Copy link
Contributor

IllusionMH commented Nov 22, 2016

I think there is no need for 100+ files change (although this level of changes probably will be required for rule test configuration files).

Probably I incorrectly used

extending RuleFailure interface with mode property

which lead to incomprehension.
I meant add mode/level property directly to this interface.

My rough estimation of changes to add level into RuleFailure .
(I'll use it instead of mode since I prefer this option and I think there were no final word on term name)

File src/language/rule/rule.ts
* Add level to IOptions
* Add level to RuleFailure constructor/toJSON
* Add getLevel method

File src/language/rule/abstractRule.ts
* Add level to this.options initialization

File src/language/walker/ruleWalker.ts
* Add private member level from options.level
* Add level to createFailure method
* Add getLevel method

File src/linter.ts
* Use filter failures array to get counts

Update formatters and utils tests (14 files) and eofline rule since they use RuleFailure constructor directly.

At this moment I'm not 100% sure if this is the best solution, since RuleWalker will store level however don't directly rely on it 😞

@olore

This comment has been minimized.

Copy link
Contributor

olore commented Nov 22, 2016

Thanks @IllusionMH ! We were on the same page. I ended up with so many changes because I couldn't get past the name RuleFailure, so I renamed it RuleViolation. Each Violation can either be an ERROR or a WARNING (aka a RuleLevel).

Hopefully this makes sense. Appreciate the feedback and suggestions.

@olore

This comment has been minimized.

Copy link
Contributor

olore commented Nov 23, 2016

After reading through some of the PRs and Issues, I think I underestimated how many custom rules are out there, and changing RuleFailure to RuleViolation will affect all of them :(

I will change back to RuleFailure

Update: 31 files changed instead of 115 👍

@nchen63

This comment has been minimized.

Copy link
Contributor

nchen63 commented Feb 7, 2017

the tests fail on my machine too. It's failing because tslint-test-custom-rules is installed intest/config/node_modules which can be found by configs in test/config while the config file for the failing test lives in test/config-legacy, which can't see the node module.

honestly, you should get rid of the config-legacy directory since it's just a copy of config but with the severity specified. It's not necessary to test every permutation of every feature with every other feature since it just adds more noise and would grow tests exponentially. Just add a few more tests to ensure that inheritance for severity is tested well.

some examples:

  • true -> severity: none
  • severity: error -> severity: warn
  • severity: error -> severity: error
  • false -> severity: none
  • true -> severity: warn

this might get complicated later on with inheriting default severity. For example, does a new default severity just override the old default or does it wipe out all of the custom levels? Do we need a "default" severity?

@olore

This comment has been minimized.

Copy link
Contributor

olore commented Feb 7, 2017

Thanks for the insight. That shouldn't be a problem. I was apprehensive about killing the legacy stuff, but it definitely felt like extra baggage.

Now you feel my pain with trying to do a default severity 😄

@nchen63

This comment has been minimized.

Copy link
Contributor

nchen63 commented Feb 7, 2017

I don't think you should necessarily kill the legacy stuff. You should actually leave it mostly unchanged since they test order of inheritance more than anything else.

@olore

This comment has been minimized.

Copy link
Contributor

olore commented Feb 16, 2017

I believe I have addressed all items. Thanks for your input.

@adidahiya

This comment has been minimized.

Copy link
Member

adidahiya commented Feb 27, 2017

ping @nchen63 -- what are the remaining actions here and what is the feature rollout plan?

@nchen63

nchen63 approved these changes Mar 1, 2017

@nchen63 nchen63 merged commit fd54825 into palantir:master Mar 1, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
cla/palantir CLA signed on 2016-11-17 23:38 UTC+00:00
Details
@nchen63

This comment has been minimized.

Copy link
Contributor

nchen63 commented Mar 1, 2017

@olore nice work

we are now on our way to 5.0!

@tugberkugurlu

This comment has been minimized.

Copy link

tugberkugurlu commented Mar 10, 2017

Has this been released?

@JKillian

This comment has been minimized.

Copy link
Contributor

JKillian commented Mar 10, 2017

@tugberkugurlu not in a regular release, but you can try it in this prerelease: https://github.com/palantir/tslint/releases/tag/5.0.0-dev.0

@prigara

This comment has been minimized.

Copy link

prigara commented Mar 23, 2017

We're working on the TSLint integration in WebStorm. Can you please point me to a comment or code describing the difference between warn, warning and none. Thanks!

@nchen63

This comment has been minimized.

Copy link
Contributor

nchen63 commented Mar 23, 2017

warn and warning are the same thing. none means that the rule will be ignored

eps1lon added a commit to eps1lon/tslint that referenced this pull request Aug 3, 2018

docs(formatters): Update stylish example
palantir#1738 added support rule severity but forgot to update the example for the stylish formatter.

@eps1lon eps1lon referenced this pull request Aug 3, 2018

Merged

docs(formatters): Update stylish example #4095

1 of 4 tasks complete

giladgray added a commit that referenced this pull request Aug 8, 2018

docs(formatters): Update stylish example (#4095)
#1738 added support rule severity but forgot to update the example for the stylish formatter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment