refactor: general rename/clean up rules #754

rafaelss95 · 2019-02-10T21:55:59Z

Current name	Proposed name
contextual-life-cycle	contextual-lifecycle
no-conflicting-life-cycle-hooks	no-conflicting-lifecycle
no-life-cycle-call	no-lifecycle-call
use-life-cycle-interface	use-lifecycle-interface
decorator-not-allowed	contextual-decorator
enforce-component-selector	use-component-selector
no-output-named-after-standard-event	no-output-native
use-host-property-decorator	no-host-metadata-property
use-input-property-decorator	no-inputs-metadata-property
use-output-property-decorator	no-outputs-metadata-property
no-queries-parameter	no-queries-metadata-property
pipe-impure	no-pipe-impure
use-view-encapsulation	use-component-view-encapsulation
i18n	template-i18n
banana-in-box	template-banana-in-box
no-template-call-expression	template-no-call-expression
templates-no-negated-async	template-no-negated-async
trackBy-function	template-use-track-by-function

Points:

1) EnforceComponentSelector -> UseComponentSelector: I decided to rename it to make it sync with other "use" rules... let me know what do you think about it.
2) NoOutputNamedAfterStandardEventRule -> NoOutputNative: not sure if it's a good name, please don't hesitate to suggest one :)
3) TemplatesNoNegatedAsync -> TemplateNoNegatedAsync: I'm not sure renaming it is a good idea, because in addition to this rule returning some false positives, it's not useful at all (see #615 (comment)) resolved in #615.
4) Since we are renaming, what about those below?

pipe-impure -> no-pipe-impure
use-host-property-decorator -> no-host-parameter (to be sync with no-queries-parameter)
use-input-property-decorator -> no-inputs-parameter (to be sync with no-queries-parameter)
use-output-property-decorator -> no-outputs-parameter (to be sync with no-queries-parameter)

Closes #653.

mgechev · 2019-02-11T15:05:00Z

This will require schematics transform of tslint.json when users execute ng update.

We'll have to discuss the change with @alexeagle and @hansl.

mgechev · 2019-02-13T23:13:59Z

README.md

+    "use-component-selector": true,
+    "use-host-property-decorator": true,
+    "use-input-property-decorator": true,
+    "use-lifecycle-interface": true,


We use this internally. Need to make sure we update g3.

mgechev · 2019-02-13T23:18:07Z

src/contextualLifecycleRule.ts

+    super.visitNgPipe(controller, decorator);
+  }
+
+  private validateDecorator(


Good job with this refactoring!

mgechev

Fantastic job! I reviewed half of the PR, I'll review the rest tomorrow.

mgechev · 2019-02-13T23:21:35Z

The renames you suggest make sense.

mgechev · 2019-02-13T23:45:15Z

src/noConflictingLifecycleHooksRule.ts

+  private validateInterfaces(node: ClassDeclaration): void {
+    const declaredLifecycleInterfaces = getDeclaredLifecycleInterfaces(node);
+
+    for (const key of Object.keys(LIFECYCLE_INTERFACE_CONFLICT_MAPPER)) {


Let's drop the for loop here, we don't really need it, nor LIFECYCLE_INTERFACE_CONFLICT_MAPPER

mgechev · 2019-02-13T23:45:49Z

src/noConflictingLifecycleHooksRule.ts

+  private validateMethods(node: ClassDeclaration): void {
+    const declaredLifecycleMethods = getDeclaredLifecycleMethods(node);
+
+    for (const key of Object.keys(LIFECYCLE_METHOD_CONFLICT_MAPPER)) {


mgechev · 2019-02-13T23:47:06Z

src/noLifecycleCallRule.ts

+  }
+}
+
+export class ExpressionCallMetadataWalker extends NgWalker {


Do we need to export this?

Not necessarily, however, for some reason, we export the walker's in almost all rules. I'll remove the export here and in other files in this PR.

mgechev · 2019-02-13T23:57:20Z

src/templateBananaInBoxRule.ts

+import { NgWalker } from './angular/ngWalker';
+import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor';
+
+const INVALID_BOX = /\[(.*)\]/;


/\[(.*)\]/.test('[(ngModel)]') && /\[(.*)\]/.test('([ngModel])') which seems incorrect.

You're right, however, the previous regex (which one is different only by a question mark) was also wrong, it gave the same result for the tests you wrote above.

After investigating why the specs pass even with this wrong regex, I discovered that the ast's name just cuts the parens, so:

Test 1: <input type="text" name="foo" ([ngModel])="foo">, test with regex returns true (the ast's name is just [ngModel]);
Test 2: <div (emitter)="emitter"></div> -> test with regex returns false (ast's name is emitter);
Test 3: <input type="text" name="foo" [ngModel]="foo"> -> test with regex returns false (ast's name is ngModelChange);
Test 4: <input type="text" name="foo" [(ngModel)]="foo"> -> never call visitEvent method.

I changed the regex a bit to: /^\[(?!$)(.*)(?<!$)\]$/.

Yes, previous regex was absolutely wrong, I noticed that. Thanks for the update!

yes @rafaelss95 , this rule used visitEvent, what explains this behavior.

mgechev · 2019-02-14T02:08:35Z

src/useComponentSelectorRule.ts

+  private validateComponent(metadata: ComponentMetadata): void {
+    if (metadata.selector) return;
+
+    const failure = getFailureMessage(metadata.controller.name!.text);


We're very confident here. Do we know for sure that name is not going to be null?

Hmm, to be honest, I really don't know if we can receive a null name here. It was copied from enforceComponentSelector, anyway, I'll add a check for this :)

mgechev · 2019-02-14T02:13:28Z

test/contextualLifecycleRule.spec.ts

@@ -0,0 +1,113 @@
+import { getFailureMessage, Rule } from '../src/contextualLifecycleRule';


I'd prefer to keep the previous specs. This is not very readable, it's hard to keep track what are the interfaces and the methods.

No problem, I'll update ASAP.

mgechev · 2019-02-14T02:14:29Z

test/noLifecycleCallRule.spec.ts

@@ -1,51 +1,46 @@
-import { lifecycleHooksMethods, LifecycleHooksMethods, Rule } from '../src/noLifeCycleCallRule';
+import { Rule } from '../src/noLifecycleCallRule';


Same here. I see we've merged the template string snippets, but I have no idea what we are testing like this. Would you mind updating? Thanks 🙇🏼‍♂️

No problem, I'll update ASAP.

mgechev

I actually had an extra hour today. Fascinating job! I love the early return refactoring and the consistency! I left a few comments. Mostly about readability of the tests.

rafaelss95 · 2019-02-14T04:17:44Z

Haha, thanks :) Btw, it's ready for the next round when you have time 💥

wKoza · 2019-02-14T08:52:33Z

README.md

+| `no-unused-css`                    | _Experimental_ |
+| `pipe-prefix`                      | _Experimental_ |
+| `template-i18n`                    | _Experimental_ |
+| `template-conditional-complexity`  | _Experimental_ |


Be careful when we'll merge this PR with #756

wKoza · 2019-02-14T09:13:32Z

src/templateBananaInBoxRule.ts

+import { NgWalker } from './angular/ngWalker';
+import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor';
+
+const INVALID_BOX = /\[(.*)\]/;


yes @rafaelss95 , this rule used visitEvent, what explains this behavior.

wKoza

Great work @rafaelss95 , just some questions

wKoza · 2019-02-14T09:20:51Z

src/contextualLifecycleRule.ts

+    decorator: Decorator,
+    allowedHookMethods: ReadonlySet<LifecycleMethodKeys>
+  ): void {
+    if (!controller.name) return;


is It possible or it's just for not ~~don't just after~~ using '!' ?

I've never seen a case where this could be null! I did it to jump the ! 😛

@Component({...}) export default class { ... }

Seems valid to me. In this case name is undefined.

hmm, In the Angular world, I'm not sure we can omit a Type. But, @mgechev , your example is interesting:
If we create a simple class like that:

export default class { ... }

and we launch ng lint, we have a big big crash [here] (https://github.com/mgechev/codelyzer/blob/master/src/propertyDecoratorBase.ts#L41) and many other places !

@mgechev , @rafaelss95 , I'm not very fan of the ! Non-Null Assertion. We cannot say that

const className = metadata.controller.name!.text

is the same that

if (!controller.name) return; const className = controllerName.text;

If we accept metadata.controller.name!.text, we should not write if (!controller.name) return;

Hence my question.

So, we must keep this test, but we must also use this test everywhere and stop to use the Non-Null Assertion.

Are you agree ?

wKoza · 2019-02-14T09:30:47Z

src/contextualLifecycleRule.ts

+    for (const member of controller.members) {
+      const { name: memberName } = member;
+
+      if (!memberName) continue;


is It possible or it's just for not ~~don't just after~~ using '!' ?

I've never seen a case where this could be null! I did it to jump the ! 😛

Let's keep it.

wKoza · 2019-02-14T11:01:02Z

test/useLifecycleInterfaceRule.spec.ts

+    describe('valid declaration of lifecycle hooks, using ng.hookName', () => {
+      it('should succeed when lifecycle hook is used with its interface', () => {
+        const source = `
+          class ${className} implements ng.OnInit {


Question: using namespace is a good practice ?

I really don't know, but I never saw someone using this in real world.

@mgechev , what do you think about it ?

This may have some implications in the dead code elimination, but it's fine to exist in our test cases.

mgechev · 2019-02-20T20:20:51Z

@rafaelss95 would you resolve the merge conflict? I think we're ready to merge.

rafaelss95 · 2019-02-20T22:42:22Z

@mgechev Sure! What about my three questions below the table?

mgechev · 2019-02-20T23:30:08Z

LGTM, the extra checks seem fine.

rafaelss95 · 2019-02-20T23:32:43Z

@mgechev I was talking about the questions in the PR description (below the table) :)

mgechev · 2019-02-21T01:32:28Z

Oh, I was sure that I responded to these questions but it looks I didn't. Yes, it makes sense to rename the rest of the rules as well.

rafaelss95 · 2019-02-24T22:16:30Z

Hey @mgechev I've updated the code and the table in OP with the new proposed names.

I took a look in (almost) all the rules today and found some more rule names that can be improved:

component-change-detection:

Suggestions:
- no-component-default-change-detection
- prefer-component-change-detection-on-push
- use-component-change-detection-on-push

relative-url-prefix:

Suggestions:
- prefer-component-relative-url-prefix
- use-component-relative-url-prefix

no-attribute-parameter-decorator:

Suggestions:
- no-attribute-decorator
- prefer-input-over-attribute-decorator

Let me know your opinion :)

mgechev · 2019-02-24T22:54:42Z

@rafaelss95 let's rename the other rules as part of another PR:

prefer-on-push-component-change-detection
no-attribute-decorator

Sound good. About the relative prefix, it's still not obvious what's about. Maybe we should be more explicit about it.

Migration of the `tslint.json` file required by the refactoring of codelyzer. For more information check this PR mgechev/codelyzer#754.

Migration of the `tslint.json` and `package.json` files required by the refactoring of codelyzer. For more information check this PR mgechev/codelyzer#754.

rafaelss95 · 2019-03-08T01:00:03Z

... About the relative prefix, it's still not obvious what's about. Maybe we should be more explicit about it.

@mgechev Do you have any suggestion?

mgechev · 2019-03-08T14:12:33Z

Let's not updated it for now.

Migration of the `tslint.json` and `package.json` files required by the refactoring of codelyzer. For more information check this PR mgechev/codelyzer#754.

- templates-use-public/no-access-missing-member (not more exist as handled by tsc - mgechev/codelyzer#260 (comment)) - invoke-injectable (not more exist as handled by ngc - mgechev/codelyzer#260 (comment)) - use-input*/use-output*/use-host* renamed - mgechev/codelyzer#754 - "typeof-compare is deprecated. Starting from TypeScript 2.2 the compiler includes this check which makes this rule redundant"

Migration of the `tslint.json` and `package.json` files required by the refactoring of codelyzer. For more information check this PR mgechev/codelyzer#754.

rafaelss95 requested review from mgechev and wKoza February 10, 2019 21:55

rafaelss95 changed the title ~~refactor: general renaming/clean up rules~~ refactor: general rename/clean up rules Feb 10, 2019

mgechev reviewed Feb 13, 2019

View reviewed changes

mgechev reviewed Feb 14, 2019

View reviewed changes

wKoza reviewed Feb 14, 2019

View reviewed changes

wKoza approved these changes Feb 20, 2019

View reviewed changes

rafaelss95 mentioned this pull request Feb 21, 2019

docs(readme): add new rules as experimental #756

Merged

mgechev mentioned this pull request Feb 21, 2019

feat(rule): heading element should have content #762

Merged

refactor: general rename/clean up rules

3f2f8c9

mgechev approved these changes Feb 24, 2019

View reviewed changes

mgechev merged commit bbf7a32 into mgechev:master Feb 24, 2019

rafaelss95 mentioned this pull request Feb 24, 2019

contextual-decorator: fails for some cases #776

Closed

rafaelss95 deleted the refactor/rename-rules branch February 25, 2019 05:21

wKoza mentioned this pull request Feb 25, 2019

Codelyzer doesn't support export default #778

Closed

mgechev added a commit to mgechev/angular-cli that referenced this pull request Mar 1, 2019

feat(@schematics/angular): tslint migration for 8

df4d561

Migration of the `tslint.json` file required by the refactoring of codelyzer. For more information check this PR mgechev/codelyzer#754.

mgechev mentioned this pull request Mar 1, 2019

feat(@schematics/angular): tslint migration for 8 angular/angular-cli#13801

Merged

mgechev added a commit to mgechev/angular-cli that referenced this pull request Mar 1, 2019

feat(@schematics/angular): tslint migration for 8

8ce7c3c

Migration of the `tslint.json` file required by the refactoring of codelyzer. For more information check this PR mgechev/codelyzer#754.

This was referenced Mar 2, 2019

refactor(rule): rename/clean up 'no-attribute-parameter-decorator' #781

Merged

refactor(rule): rename/clean up 'component-change-detection' #782

Merged

refactor(rule): rename 'max-inline-declarations' to 'component-max-inline-declarations' #785

Merged

mgechev added a commit to mgechev/angular-cli that referenced this pull request Mar 4, 2019

feat(@schematics/angular): tslint migration for 8

4bdd3e4

Migration of the `tslint.json` file required by the refactoring of codelyzer. For more information check this PR mgechev/codelyzer#754.

		@@ -0,0 +1,113 @@
		import { getFailureMessage, Rule } from '../src/contextualLifecycleRule';

		@@ -1,51 +1,46 @@
		import { lifecycleHooksMethods, LifecycleHooksMethods, Rule } from '../src/noLifeCycleCallRule';
		import { Rule } from '../src/noLifecycleCallRule';

refactor: general rename/clean up rules #754

refactor: general rename/clean up rules #754

Conversation

rafaelss95 commented Feb 10, 2019 • edited Loading

Points:

mgechev commented Feb 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgechev left a comment

Choose a reason for hiding this comment

mgechev commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelss95 Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelss95 Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgechev left a comment

Choose a reason for hiding this comment

rafaelss95 commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wKoza left a comment

Choose a reason for hiding this comment

wKoza Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wKoza Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

wKoza Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgechev commented Feb 20, 2019

rafaelss95 commented Feb 20, 2019 • edited Loading

mgechev commented Feb 20, 2019

rafaelss95 commented Feb 20, 2019

mgechev commented Feb 21, 2019

rafaelss95 commented Feb 24, 2019 • edited Loading

mgechev commented Feb 24, 2019

rafaelss95 commented Mar 8, 2019

mgechev commented Mar 8, 2019

rafaelss95 commented Feb 10, 2019 •

edited

Loading

rafaelss95 Feb 14, 2019 •

edited

Loading

rafaelss95 Feb 14, 2019 •

edited

Loading

wKoza Feb 14, 2019 •

edited

Loading

wKoza Feb 21, 2019 •

edited

Loading

wKoza Feb 14, 2019 •

edited

Loading

rafaelss95 commented Feb 20, 2019 •

edited

Loading

rafaelss95 commented Feb 24, 2019 •

edited

Loading