refactor: general rename/clean up rules by rafaelss95 · Pull Request #754 · mgechev/codelyzer

rafaelss95 · Feb 10, 2019

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 · Feb 11, 2019

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 · Feb 13, 2019

+    "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 · Feb 13, 2019

+    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 · Feb 13, 2019

The renames you suggest make sense.

mgechev · Feb 13, 2019

+  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 · Feb 13, 2019

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


mgechev · Feb 13, 2019

+  }
+}
+
+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 · Feb 13, 2019

+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 · Feb 14, 2019

+  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 · Feb 14, 2019

@@ -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 · Feb 14, 2019

@@ -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 · Feb 14, 2019

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

wKoza · Feb 14, 2019

+| `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 · Feb 14, 2019

+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 · Feb 14, 2019

+    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 · Feb 14, 2019

+    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 · Feb 14, 2019

+    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 · Feb 20, 2019

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

rafaelss95 · Feb 20, 2019

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

mgechev · Feb 20, 2019

LGTM, the extra checks seem fine.

rafaelss95 · Feb 20, 2019

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

mgechev · Feb 21, 2019

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 · Feb 24, 2019

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 · Feb 24, 2019

@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 · Mar 8, 2019

... 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 · Mar 8, 2019

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';

Search code, repositories, users, issues, pull requests...

Conversation

rafaelss95 commented Feb 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Points:

Uh oh!

mgechev commented Feb 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgechev left a comment

Choose a reason for hiding this comment

Uh oh!

mgechev commented Feb 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaelss95 Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaelss95 Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgechev left a comment

Choose a reason for hiding this comment

Uh oh!

rafaelss95 commented Feb 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wKoza left a comment

Choose a reason for hiding this comment

Uh oh!

wKoza Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wKoza Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wKoza Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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