Improve error messages for no-any and object-literal-shorthand. #2164

mprobst · 2017-02-01T11:46:39Z

Users often use any when they could equally well use {}, because
they are not aware of the empty type.

Users also often don't know what object shorthand is - giving them a
hint as to what syntax is expected makes it easier to fix the lint
warning.

PR checklist

New feature, bugfix, or enhancement
- Includes tests

What changes did you make?

Improve error messages for no-any and object-literal-shorthand.

[no-log]

mprobst · 2017-02-01T11:58:17Z

Lots of lints warnings in tslint itself (though yarn verify passes locally for me) - probably not worth fixing in this PR? The tests affected by this change pass locally.

nchen63 · 2017-02-01T13:02:52Z

can you rebase?

mprobst · 2017-02-01T13:14:17Z

@nchen63 done.

mprobst · 2017-02-01T13:20:09Z

@nchen63 not sure how to fix the build failure about requiring 4x parallelism. I've made that setting in the UI as suggested by the error, but the build still seems to fail?

IllusionMH · 2017-02-01T13:54:40Z

@mprobst have you tried "Try build" on shorthand-err branch after you set parallelism to 4? https://circleci.com/gh/mprobst/tslint/edit#parallel-builds
As I remember it started to work after few attempts for me

mprobst · 2017-02-01T14:35:09Z

@IllusionMH ah, it appears if you "Rebuild" a failed build, it'll use the same parallelism settings. Which is not that helpful if the build failed because of too little parallelism...

In any case, seems fixed now.

alexeagle · 2017-02-01T17:41:19Z

src/rules/objectLiteralShorthandRule.ts

@@ -31,8 +31,8 @@ export class Rule extends Lint.Rules.AbstractRule {
    };
    /* tslint:enable:object-literal-sort-keys */

-    public static LONGHAND_PROPERTY = "Expected property shorthand in object literal.";
-    public static LONGHAND_METHOD = "Expected method shorthand in object literal.";
+    public static LONGHAND_PROPERTY = "Expected property shorthand in object literal ('{foo, bar}').";


I doubt all users will understand this, because it's not obvious that foo is a property name (it also looks syntactically like destructuring). Easy fix is print {prop1, prop2} instead so the names are self-documenting.
Ideally we should point to the https://palantir.github.io/tslint/rules/object-literal-shorthand/ page, and make that have more information explaining the syntax for newbies (they may not be aware of the shorthand syntax at all)

alexeagle · 2017-02-01T17:41:31Z

src/rules/objectLiteralShorthandRule.ts

-    public static LONGHAND_PROPERTY = "Expected property shorthand in object literal.";
-    public static LONGHAND_METHOD = "Expected method shorthand in object literal.";
+    public static LONGHAND_PROPERTY = "Expected property shorthand in object literal ('{foo, bar}').";
+    public static LONGHAND_METHOD = "Expected method shorthand in object literal ('{foo() {...}}').";


this would be a great check to add a suggested fix

I have a different PR for that, though only for the shorthand property - the functions are tricky to find in the AST.

alexeagle · 2017-02-01T17:45:42Z

src/rules/noAnyRule.ts

@@ -33,7 +33,7 @@ export class Rule extends Lint.Rules.AbstractRule {
    };
    /* tslint:enable:object-literal-sort-keys */

-    public static FAILURE_STRING = "Type declaration of 'any' is forbidden";
+    public static FAILURE_STRING = "Type declaration of 'any' is forbidden (use the empty type '{}'?)";


I think users will still stumble here. Linking to the doc would be best (see other comment)

Wordsmith attempt:
Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type, the empty type ('{}'), or suppress this occurrence

Separately, it would be nice if the end of a tslint failure report pointed you to the docs for suppression https://palantir.github.io/tslint/usage/rule-flags/ - nearly every user I've coached was unaware of those.

Separately, it would be nice if the end of a tslint failure report pointed you to the docs for suppression https://palantir.github.io/tslint/usage/rule-flags/ - nearly every user I've coached was unaware of those.

I don't think bloating the failure message even more adds any more value to it. Most of the time you see it as a tooltip in your editor and cannot even click the link.

Some (most?) IDEs with tslint integration show you a "quickfix" option at rule failures that allows you to automatically disable that rule for that line. At least that's what vscode, IntelliJ and friends do.

We have a lot of novice users, who are mailing out their PR for review, and their command-line (or even an email) reports that they have to stop and go back to fix something. For these users, there is plenty of space for more text, and this is the right UX to also explain to a novice user what they did wrong and how to fix it.

IMO the core problem here is we have one string field which must fit into a tooltip, in an IDE environment where there is other help, and also be used in the command-line interface where the context is missing.

Maybe on the command line we could also print the Description field? Though I know most descriptions were not written with this in mind. Maybe add a new field "LONG_FAILURE_STRING" or similar? @jkillian ?

I went with yours @alexeagle, @ajafff @nchen63 let me know what you think.

Overall my impression is that we won't be able to enable this rule in practice if people have no idea how to react. So even if the long text is slightly sub optimal in IDEs, it's still better than having a rule that you cannot really use in production.

I think the updated text has the right amount of verbosity

Users often use `any` when they could equally well use `{}`, because they are not aware of the empty type. Users also often don't know what object shorthand is - giving them a hint as to what syntax is expected makes it easier to fix the lint warning.

nchen63 · 2017-02-09T03:56:48Z

@mprobst thanks!

mprobst force-pushed the shorthand-err branch from 3a93a92 to 277b61b Compare February 1, 2017 13:13

alexeagle reviewed Feb 1, 2017

View reviewed changes

adidahiya assigned nchen63 Feb 1, 2017

mprobst added 3 commits February 7, 2017 16:51

Make the no-any rule more verbose.

d226e22

Include actual syntax suggestions in the objectLiteralShorthand rule.

fc40205

mprobst force-pushed the shorthand-err branch from 4573292 to fc40205 Compare February 7, 2017 15:52

nchen63 merged commit e2a4c23 into palantir:master Feb 9, 2017

mprobst deleted the shorthand-err branch February 9, 2017 13:37

renovate-bot mentioned this pull request Feb 27, 2017

Update dependency tslint to version 4.5.1 singapore/lint-condo#251

Merged

ChrisMBarr mentioned this pull request Apr 4, 2017

Bad suggestions from the no-any rule #2491

Closed

rrdelaney mentioned this pull request Mar 10, 2020

Upgrade to TypeScript 3.8 #4915

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error messages for no-any and object-literal-shorthand. #2164

Improve error messages for no-any and object-literal-shorthand. #2164

mprobst commented Feb 1, 2017 •

edited by nchen63

Loading

mprobst commented Feb 1, 2017

nchen63 commented Feb 1, 2017

mprobst commented Feb 1, 2017

mprobst commented Feb 1, 2017

IllusionMH commented Feb 1, 2017

mprobst commented Feb 1, 2017

alexeagle Feb 1, 2017

mprobst Feb 2, 2017

alexeagle Feb 1, 2017

mprobst Feb 2, 2017

alexeagle Feb 1, 2017

ajafff Feb 1, 2017

alexeagle Feb 1, 2017

mprobst Feb 2, 2017 •

edited

Loading

nchen63 Feb 7, 2017

nchen63 commented Feb 9, 2017

Improve error messages for no-any and object-literal-shorthand. #2164

Improve error messages for no-any and object-literal-shorthand. #2164

Conversation

mprobst commented Feb 1, 2017 • edited by nchen63 Loading

PR checklist

What changes did you make?

mprobst commented Feb 1, 2017

nchen63 commented Feb 1, 2017

mprobst commented Feb 1, 2017

mprobst commented Feb 1, 2017

IllusionMH commented Feb 1, 2017

mprobst commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mprobst Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nchen63 commented Feb 9, 2017

mprobst commented Feb 1, 2017 •

edited by nchen63

Loading

mprobst Feb 2, 2017 •

edited

Loading