Skip to content
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

Merged
merged 1 commit into from May 10, 2018
Merged

fix: some rules not considering options correctly #617

merged 1 commit into from May 10, 2018

Conversation

rafaelss95
Copy link
Collaborator

The following rules aren't behaving correctly at the moment:

  • max-inline-declarations;
  • no-input-prefix;
  • prefer-inline-decorator;

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:

{
  "rules": {
    "max-inline-declarations": [true, { "template": 2 }]
  }
}
@Component({
  selector: 'app-root',
  template: `
    <div>
     something here
    </div>
  `,
  styleUrls: ['./app.component.css']
})
export class AppComponent {}

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.

@rafaelss95
Copy link
Collaborator Author

@wKoza can you review?

@wKoza
Copy link
Collaborator

wKoza commented May 9, 2018

@rafaelss95 , we have an another problem with the rule max-inline-declarations in your example. This rule compute all the lines between ' ... '. Your use case, the rule count 5 lines but I think that 3 is the good value. So, we should exclude a first and the last line when it is empty lines.

@@ -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}`;
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

@rafaelss95
Copy link
Collaborator Author

rafaelss95 commented May 9, 2018

@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 max-file-line-count not count comment lines.

@Component({
  selector: 'app-root',
  template: `
    <div>
     something here
     <!-- a comment here -->
    </div>
  `,
  styleUrls: ['./app.component.css']
})
export class AppComponent {}

@wKoza
Copy link
Collaborator

wKoza commented May 10, 2018

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.

@wKoza wKoza merged commit bce0026 into mgechev:master May 10, 2018
@rafaelss95 rafaelss95 deleted the fix-tests branch May 10, 2018 09:49
@mgechev mgechev added this to the 4.4.0 - Ken Thompson milestone Jun 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants