-
Notifications
You must be signed in to change notification settings - Fork 235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: some rules not considering options correctly #617
Conversation
@wKoza can you review? |
@rafaelss95 , we have an another problem with the rule |
src/maxInlineDeclarationsRule.ts
Outdated
@@ -59,8 +88,9 @@ export class MaxInlineDeclarationsValidator extends NgWalker { | |||
private validateInlineTemplate(metadata: ComponentMetadata): void { | |||
if (this.hasInlineTemplate(metadata) && this.getTemplateLinesCount(metadata) > this.templateLinesLimit) { | |||
const templateLinesCount = this.getTemplateLinesCount(metadata); | |||
const msg = `Inline template lines limit exceeded. Defined limit: ${this.templateLinesLimit} / template lines: ${templateLinesCount}`; |
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.
It's a shame to lost the value : template lines
. It's a valuable value.
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.
No problem, I'll put it again.
@wKoza Hmm, yes, I noticed this problem now. I've pushed the changes you requested. Thinking more about it... should we exclude all empty lines from calculation or only the first and last? And what about comments? I'm asking because someone, for example, requested in palantir/tslint#3672 to @Component({
selector: 'app-root',
template: `
<div>
something here
<!-- a comment here -->
</div>
`,
styleUrls: ['./app.component.css']
})
export class AppComponent {} |
The main reason for this rule is to increase readabilty of our components. All the HTML source code obscure the component's implementation, so, I think we can compute comments and the other empty lines. |
The following rules aren't behaving correctly at the moment:
The problem with these rules are that it's only considering the options from the second element (in position 1) of the
array
, which is totally incorrect.Example for
max-inline-declarations
:Expected behavior:
Failure.
Actual behavior:
No failure.
This fixes the wrong behavior and respective tests, with some refactor.
PS: The spec file of
max-inline-declarations
was wrongly named.