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

Object literal shorthand rule #1488

Merged
merged 7 commits into from
Aug 19, 2016
Merged

Conversation

danvk
Copy link
Contributor

@danvk danvk commented Aug 16, 2016

Closes #304

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @danvk! 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.

@adidahiya
Copy link
Contributor

@danvk can we make the name a little more verbose, object-literal-shorthand? I don't really like eslint's name for this rule.

also thanks for signing the CLA. as you can see that's a new thing :)

@@ -35,4 +35,15 @@ describe("Utils", () => {
assert.deepEqual(objectify(1), {});
assert.deepEqual(objectify({foo: 1, mar: {baz: 2}}), {foo: 1, mar: {baz: 2}});
});

it("dedent", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

};

const quotes = {
"foo-bar": function() {},
Copy link
Contributor

Choose a reason for hiding this comment

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

what about when the function is named? what's the expected behavior?

@adidahiya adidahiya changed the title Object shorthand Object literal shorthand rule Aug 16, 2016
@adidahiya
Copy link
Contributor

some of my comments got collapsed in an outdated diff but they're still relevant -- please click through

@danvk
Copy link
Contributor Author

danvk commented Aug 16, 2016

Thanks for the quick review @adidahiya. I've addressed all your comments. Good catch on named function expressions. eslint allows them and now I do, too.

@danvk
Copy link
Contributor Author

danvk commented Aug 16, 2016

At the risk of side-tracking this PR, I am curious what you guys think of https://github.com/eslint/typescript-eslint-parser as an alternative way of running lint rules like this one that aren't TS-specific. It would be great to use the AirBnb style guide with TS.

@danvk
Copy link
Contributor Author

danvk commented Aug 19, 2016

ping @adidahiya @jkillian anything else you need from me on this?

@jkillian jkillian merged commit 3a24378 into palantir:master Aug 19, 2016
@jkillian
Copy link
Contributor

Looks good, thanks @danvk! I haven't given the ESLint parser a try yet, have you had any luck with it? I believe it strips all TS-specific elements from the AST?

@danvk
Copy link
Contributor Author

danvk commented Aug 19, 2016

@jkillian I believe that's correct. You should really take typescript-eslint-parser for a spin. It's quite easy to set up and gives a mixture of great and less-great results.

  • For style rules (e.g. object-curly-spacing, no-extra-semi, space-infix-ops, indent, padded-blocks, ...) it's perfect.
  • Other rules like no-undef, no-unused-vars and dot-notation, it's hopeless. You can import symbols exclusively to use them as types, which it will never be able to understand.
  • Rules that are purely about ES6 semantics also work well, e.g. guard-for-in, no-restricted-syntax and prefer-arrow-callback.

I think some sort of combination of eslint+typescript-eslint-parser and tslint could be very powerful. There's no sense in replicating all the eslint rules that are independent of TS (like space-infix-ops). But there are many rules that eslint can't get right in a TypeScript context.

@jkillian
Copy link
Contributor

Wrapping ESLint in some way and using its style rules is an interesting idea. Do the style rules actually work that well though? For example, does the AST that ESLint builds include interface definitions or generic constructs, both elements that wouldn't be normally found in an ES AST?

@danvk
Copy link
Contributor Author

danvk commented Aug 25, 2016

The rules that are purely about style (whitespace / indentation / variable naming) mostly work great.

But unless their plan is different than what I think, there will always be some rules they can't implement because they require type information, e.g. no-undef. tslint will always be needed for those sorts of rules.

Another nice thing about the typescript-eslint-parser approach is that you get eslint --fix for free.

@adidahiya
Copy link
Contributor

@danvk there's discussion going on here about this: eslint/typescript-eslint-parser#61

@JamesHenry
Copy link

JamesHenry commented Aug 25, 2016

Thanks so much for linking to that issue, @adidahiya, and thanks for trying out the parser @danvk!

Currently, that thread is definitely the best place to learn about how we feel the combination of the typescript-eslint-parser and eslint-typescript-plugin will be able to facilitate full linting of TypeScript code with ESLint.

I appreciate that it is not necessarily very clear from the existing documentation what the scope of the project is. We will definitely rectify that once we get closer to a full "beta".

Please note that there is no limit to the ambition here, so for example @danvk:

But unless their plan is different than what I think, there will always be some rules they can't implement

...it most definitely is different! You can see that we have already debunked the false preconceptions about what is possible, and not possible, in that issue.

@jkillian:

For example, does the AST that ESLint builds include interface definitions or generic constructs

Yes, it already does! You can see how the AST generated from this source https://github.com/eslint/typescript-eslint-parser/blob/master/tests/fixtures/typescript/basics/class-with-implements-generic-multiple.src.ts includes typeParameters here: https://github.com/eslint/typescript-eslint-parser/blob/master/tests/fixtures/typescript/basics/class-with-implements-generic-multiple.result.js#L107

Things are progressing very well, it is simply early on in the process.

At this stage, we are able to parse all stable TypeScript (i.e. currently 1.8.10) syntax (including TSX). In addition to that, we do already have some type information recorded in the ESTree-compatible AST (matching Flow's terminology, and as linked above) and I am most of the way through adding full decorator support.

We are also currently spec'ing out how we can elegantly support things such as the full TypeScript TypeChecker being used within rules (you can already see a POC of what those kind of rules could do in that thread).

Thanks again for taking the interest in the parser, and for your work on TSLint. Everybody in the TypeScript community is going to benefit from having both TSLint and ESLint available to them!

@jkillian
Copy link
Contributor

Thanks for the detailed response @JamesHenry! I responded in part over in eslint/typescript-eslint-parser#61 - it seemed easiest to keep discussion in one thread over there

soniro pushed a commit to soniro/tslint that referenced this pull request Aug 31, 2016
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.

5 participants