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

Add "check-type-operator" option for whitespace rule #3083

Merged
merged 1 commit into from
Aug 11, 2017
Merged

Add "check-type-operator" option for whitespace rule #3083

merged 1 commit into from
Aug 11, 2017

Conversation

santialbo
Copy link
Contributor

@santialbo santialbo commented Jul 29, 2017

PR checklist

Overview of change:

This PR adds the "check-type-operator" option to the whitespace rule. The implementation is very similar to the binary operator one.

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

CHANGELOG.md entry:

[new-rule-option] check-type-operator for whitepace rule

@palantirtech
Copy link
Member

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

@ajafff
Copy link
Contributor

ajafff commented Aug 1, 2017

This change looks very familiar to me. I think I've seen a similar PR before, but cannot find it right now.

Btw. that's not a type operator (IIRC keyof T is a type operator - at least in the AST)

@santialbo
Copy link
Contributor Author

@ajafff
Copy link
Contributor

ajafff commented Aug 1, 2017

Got it: #2884

@santialbo
Copy link
Contributor Author

Yup, it does pretty much the same. Looking at the changes I noticed the other PR is not checking for the option to be true.
Also the reason I called them type operators is because of the rule in the typescript repo I mentioned before.

@ajafff
Copy link
Contributor

ajafff commented Aug 1, 2017

You're right. Added a comment over there.

Re type operator: unfortunately that could lead to confusion with the typescript spec where type operator refers to something completely different

@adidahiya
Copy link
Contributor

The spec currently refers to them as operators, so I think this naming is better than "type list".

@santialbo
Copy link
Contributor Author

Rebased with latest master, I think the CI error is unrelated to this PR.

@@ -9,7 +9,8 @@
"check-separator",
"check-rest-spread",
"check-type",
"check-typecast"
"check-typecast",
"check-type-operator"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also want to add this to tslint:latest?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes please

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll do it in a separate PR

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

your code looks good @santialbo, I'm fixing the master build here #3118

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.

4 participants