-
Notifications
You must be signed in to change notification settings - Fork 889
Conversation
@jkillian @adidahiya This is ready for review. |
Sorry for the delay @danvk, was away from the office. I'll try to give this a review by the end of the week at the latest |
/* tslint:disable:object-literal-sort-keys */ | ||
public static metadata: Lint.IRuleMetadata = { | ||
ruleName: "quote-props", | ||
description: "Quoting Style for Property Names", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Enforces consistent object literal property quote style."
I think the above would be more consistent with the current descriptions.
Looking good! Just found a few documentation tweaks to make if you don't mind |
Addressed all comments, thanks for the review! |
} | ||
} | ||
|
||
// This is simplistic. See https://mothereff.in/js-properties for the gorey details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol this url domain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the whole URL works pretty well for this topic! :)
@danvk small note for future PRs: it makes it easier for us to review if you address CR comments in a follow-up commit (Github's squash+merge feature makes it easy to squash revision commits at the end). Thanks again for the contribution! |
(I often use tools like Reviewable, which have a notion of revisions/snapshots independent from the current commits.) |
(split into two commits so you guys can see the diff more easily.) |
Thanks! Last change, could we rename the rule to |
@jkillian do you guys care about maintaining parity with eslint? That's where the |
@danvk That's definitely something that's taken into consideration. However, I feel like ESLint rule names can be a little bit hit or miss, so I'm leaning away from worrying about parity too much. I'd like to get better naming guidelines formalized (#1371), so if you have overall input, would be glad to hear your thoughts. |
|
@jkillian this should be good to go |
Thanks @danvk, merged! |
* Core quote-props rule * Tweaks for code review * rename
This adds a tslint equivalent to a subset of the eslint
quote-props
rule (see #1260).The eslint rule is fairly elaborate. I've implemented the "as-needed" and "always" behaviors: http://eslint.org/docs/rules/quote-props
I verified that my tests produce the same results as eslint for the same code and settings.
There may be some incorrect behavior around things like unicode property names, but this is a good start.