New Rule: param-property-in-constructor #1358
Conversation
|
Sorry about the merge commits. I thought github desktop was better than that! I'll do it from the command line in the future. |
|
How do you feel about the name |
|
|
||
| private languageService: ts.LanguageService; | ||
|
|
||
| constructor(sourceFile: ts.SourceFile, options: Lint.IOptions, languageService: ts.LanguageService) { |
There was a problem hiding this comment.
You could just make this private languageService: ts.LanguageService if you want (funny considering what this rule does 😄 )
There was a problem hiding this comment.
lol, ugh why did I not notice this! 😆
| public visitConstructorDeclaration(node: ts.ConstructorDeclaration) { | ||
| if (node.parameters && node.parameters.length > 0) { | ||
| const fileName = this.getSourceFile().fileName; | ||
| node.parameters.forEach((param: ts.ParameterDeclaration) => { |
There was a problem hiding this comment.
for-of usage is preferred when you don't need an index argument. (I think this is for performance reasons?)
There was a problem hiding this comment.
Performance and consistency (one iteration style)
|
The test failures seem to be related to changes that were made before this branch was started. I think it would be fine to merge into master, but please double check me. Also ignore my commit on a new branch there, i was trying to simplify things but accidentally made it more complex. so just ignore that extra branch. |
adidahiya
changed the title
|
Can you merge master here as well, so we can see the CI passing before merging this PR? |
|
|
||
| class FailingExample2 { | ||
| constructor(public thing: Object) { | ||
| const wow=thing.doTheThing() |
There was a problem hiding this comment.
Hmm, I wasn't thinking about this use case. When accessing a value, it doesn't matter if you use this. or not. Do you think we should still disallow this?
There was a problem hiding this comment.
On the other hand, using this. doesn't hurt
There was a problem hiding this comment.
yeah that was my thinking with the way it was written initially. I was thinking that if we just require this. on everything, then there is consistency for how all constructor params are used, regardless if they are used as properties or not.
|
Hmm, actually looks like TS has some sort of similar functionality build in: microsoft/TypeScript#10062 |
|
@jkillian You're right. We can validate unused parameter property with |
|
related: #1481 |
|
I don't think the benefits of this rule are worth it given the features in TS 2.0. Sorry @ChrisMBarr! Let me know if you strongly disagree |
|
@jkillian :Some code I saw today in a project lead me back here to see why this PR was never accepted. Reading over the above comments, I'm not entirely sure now. The intent of this PR was not to find unused constructor parameters, it was meant to enforce the correct usage of those parameters. constructor(public foo: number) {
this.foo = 10;
foo = 5; //<-- violation
}I can re-submit this PR, but I don't want to put the work in for it unless it's desired. |
New rule created based on discussions in #1336
The rule is named
no-improper-constructor-param-usageand has no configuration options.Here’s a quick code sample of what will trigger a violation