[enhancement] object-literal-shorthand / config to disallow shorthand #3268

aervin · 2017-10-01T04:18:04Z

PR checklist

Addresses an existing issue: #2953
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Can pass argument "never" to object-literal-shorthand rule to disallow shorthand syntax. Currently, default is "always".

ajafff · 2017-10-01T13:13:43Z

src/rules/objectLiteralShorthandRule.ts

    }
 }

-function walk(ctx: Lint.WalkContext<void>) {
+function walk(ctx: Lint.WalkContext<Options>) {


The branches for both options don't share any code. Consider splitting into two separate functions. That will make it easier to understand and avoids unnecessary CPU cycles.

ajafff · 2017-10-01T13:14:25Z

src/rules/objectLiteralShorthandRule.ts

-        if (isPropertyAssignment(node)) {
-            if (node.name.kind === ts.SyntaxKind.Identifier &&
+        if (isShorthandAssignment(node) && !allowShorthandAssignments) {
+            ctx.addFailureAtNode(node, Rule.SHORTHAND_ASSIGNMENT);


fixing the shorthand to longhand syntax shouldn't be too hard I guess

ajafff · 2017-10-01T13:15:29Z

src/rules/objectLiteralShorthandRule.ts

+    name: ts.PropertyName,
+    initializer: ts.FunctionExpression,
+    sourceFile: ts.SourceFile,
+): [string, Lint.Fix] {


Please avoid all these purely stylistic changes. They don't add any value to the PR and make the diff harder to reason about.

I think I have a TS formatter formatting on save. I'll take care of it.

… style changes

ajafff · 2017-10-02T12:24:24Z

src/rules/objectLiteralShorthandRule.ts

+function fixShorthandToLonghand(node: ts.ShorthandPropertyAssignment | ts.MethodDeclaration): Lint.Fix {
+    let replacementText =   isMethodDeclaration(node)
+                                ? ": function"
+                                : `: ${node.name.getText()}`;


you can use node.name.text here

ajafff · 2017-10-02T12:28:00Z

src/rules/objectLiteralShorthandRule.ts

-function walk(ctx: Lint.WalkContext<void>) {
+function walk(ctx: Lint.WalkContext<Options>) {
+    const { options: { allowShorthandAssignments } } = ctx;
+    allowShorthandAssignments ? enforceShorthand(ctx) : disallowShorthand(ctx);


you can also do that in Rule#apply:

this.applyWithFunction(sourceFile, this.ruleArguments.indexOf(OPTION_NEVER) === -1 ? enforceShorthand : disallowShorthand);

ajafff · 2017-10-02T15:34:32Z

src/rules/objectLiteralShorthandRule.ts

@@ -63,6 +103,19 @@ function walk(ctx: Lint.WalkContext<void>) {
    });
 }

+function fixShorthandToLonghand(node: ts.ShorthandPropertyAssignment | ts.MethodDeclaration): Lint.Fix {
+    let replacementText = isMethodDeclaration(node) ? ": function" : `: ${node.name.getText()}`;


you can substitute node.name.getText() with node.name.text

ajafff · 2017-10-02T15:35:53Z

test/rules/object-literal-shorthand/never/test.ts.lint

+
+const asyncFn = {
+  foo: async function() {},
+  bar: async function*() {}


needs a test for async shorthand

ajafff · 2017-10-02T15:40:50Z

src/rules/objectLiteralShorthandRule.ts

+    if (isGeneratorFunction(node)) {
+        const asteriskPosition = (node as ts.MethodDeclaration).asteriskToken!.getStart();
+        fixes.push(Lint.Replacement.replaceFromTo(asteriskPosition, asteriskPosition + 1, ""));
+    }


needs special handling for async methods as well.

before you do that, just a suggestion to simplify the fix: replace the name with the function keyword and insert the name plus the colon at the start of the declaration.
That way you don't have to handle the async keyword or the asterisk at all

ajafff · 2017-10-02T16:57:55Z

src/configs/all.ts

@@ -204,7 +204,7 @@ export const rules = {
    "no-unnecessary-type-assertion": true,
    "number-literal-format": true,
    "object-literal-key-quotes": [true, "consistent-as-needed"],
-    "object-literal-shorthand": true,
+    "object-literal-shorthand": [true, "always"],


With the current implementation "always" is not a valid option.

aervin · 2017-10-02T17:45:46Z

I'm struggling to simplify the fixer function b/c of this case:

var obj4 = {
  async* f() {
   yield 1;
   yield 2;
   yield 3;
  }
};

ajafff

almost done

ajafff · 2017-10-02T19:52:17Z

src/rules/objectLiteralShorthandRule.ts

+    let
+    replacementStart = (isAsync && node.modifiers !== undefined) ? getModifier(node, ts.SyntaxKind.AsyncKeyword)!.getStart() : -1;
+    replacementStart = (isGenerator && !isAsync) ? (node as ts.MethodDeclaration).asteriskToken!.getStart() : -1;
+    replacementStart = replacementStart === -1 ? node.name.getStart() : replacementStart;


the async keyword is the only modifier allowed here, so replacementStart can always be node.getStart()

ajafff · 2017-10-02T19:53:59Z

src/rules/objectLiteralShorthandRule.ts

+                    `${node.name.getText()}:${isAsync ? " async" : ""} function${isGenerator ? "*" : ""}`,
+                )
+            /* tslint:disable:trailing-comma */
+            : Lint.Replacement.appendText(node.name.getStart(), `${node.name.text}: `)


consider moving the fix for ShorthandPropertyAssignment out of the function. It will make many conditions and type assertions unnecessary

ajafff · 2017-10-02T19:54:44Z

src/rules/objectLiteralShorthandRule.ts

+function isShorthandAssignment(node: ts.Node): boolean {
+    return (
+        isShorthandPropertyAssignment(node) ||
+        (isMethodDeclaration(node) && node.parent !== undefined ? node.parent.kind === ts.SyntaxKind.ObjectLiteralExpression : false)


node.parent cannot be undefined
also no need for the conditional expression, just use &&

ajafff · 2017-10-02T19:56:26Z

test/rules/object-literal-shorthand/never/test.ts.lint

+    x: x
+  }
+};
+[OBJECT_LITERAL_DISALLOWED]: Shorthand property assignments have been disallowed.


please add a test case with class methods. these should be ignored by the rule

You mean class members which instantiate object literals with shorthand props?

I mean regular method declarations inside a class. They are ignored by the rule. I just want a test to ensure this won't regress.

class C { doStuff() {} // shouldn't be an error }

Ahh of course. Thanks

…s methods

ajafff

LGTM with a suggestion

ajafff · 2017-10-03T14:53:52Z

src/rules/objectLiteralShorthandRule.ts

+    return ts.forEachChild(ctx.sourceFile, function cb(node): void {
+        if (isShorthandAssignment(node)) {
+            ctx.addFailureAtNode(
+                node,


looking at the tests, I think it's better to just show the error at node.name

Went ahead and updated the error messages for shorthand-only too

ajafff · 2017-10-03T15:33:03Z

Went ahead and updated the error messages for shorthand-only too

I was just about to comment on that. I think showing the error only at the name is a bit too narrow for those errors.
If you revert that, I think this PR is good to go. Sorry for the back and forth.

aervin · 2017-10-03T15:37:46Z

Haha maybe a bit presumptuous on my part. Anyway, we made it! Thanks as always for your help/patience.

ajafff · 2017-10-03T15:43:04Z

Thanks @aervin

…palantir#3268)

Config for object-literal-shorthand init implementation

b1a3c8b

ajafff suggested changes Oct 1, 2017

View reviewed changes

Aaron Ervin added 3 commits October 1, 2017 11:30

separate functions for enforcing/disallowing shorthand; tried to undo…

524bd38

… style changes

fixer for disallowed obj lit shorthand

c0ea4ff

undo formatting to make PR easier to read

248b8d1

ajafff reviewed Oct 2, 2017

View reviewed changes

addresses async shorthand; strange trailing-comma bug?

7b52c56

ajafff reviewed Oct 2, 2017

View reviewed changes

tried to simplify fixer; removed unnec casts, etc; test case for clas…

fbdf337

…s methods

ajafff approved these changes Oct 3, 2017

View reviewed changes

updated rule's lint error messages node -> node.name

46a4c59

reverted changes to always tests

1b6ac0f

ajafff merged commit dfc444b into palantir:master Oct 3, 2017

This was referenced Oct 3, 2017

make object-literal-shorthand configurable #2953

Closed

object-literal-shorthand: various optimizations #3273

Merged

HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018

[enhancement] object-literal-shorthand / config to disallow shorthand (…

7a9579c

…palantir#3268)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[enhancement] object-literal-shorthand / config to disallow shorthand #3268

[enhancement] object-literal-shorthand / config to disallow shorthand #3268

aervin commented Oct 1, 2017

ajafff Oct 1, 2017

ajafff Oct 1, 2017

ajafff Oct 1, 2017

aervin Oct 1, 2017

ajafff Oct 2, 2017

ajafff Oct 2, 2017

ajafff Oct 2, 2017

ajafff Oct 2, 2017

ajafff Oct 2, 2017

ajafff Oct 2, 2017

aervin commented Oct 2, 2017

ajafff left a comment

ajafff Oct 2, 2017

ajafff Oct 2, 2017

ajafff Oct 2, 2017

ajafff Oct 2, 2017

aervin Oct 2, 2017

ajafff Oct 2, 2017

aervin Oct 2, 2017

ajafff left a comment

ajafff Oct 3, 2017

aervin Oct 3, 2017

ajafff commented Oct 3, 2017

aervin commented Oct 3, 2017

ajafff commented Oct 3, 2017

[enhancement] object-literal-shorthand / config to disallow shorthand #3268

[enhancement] object-literal-shorthand / config to disallow shorthand #3268

Conversation

aervin commented Oct 1, 2017

PR checklist

Overview of change:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aervin commented Oct 2, 2017

ajafff left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff commented Oct 3, 2017

aervin commented Oct 3, 2017

ajafff commented Oct 3, 2017