void-0-rule #770

lizzzp1 · 2019-01-07T03:55:39Z

PR checklist

Addresses an existing issue: fixes Rule suggestion: void-zero #693
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Attempts to add a no void 0 rule.

Is there anything you'd like reviewers to focus on?

@JoshuaKGoldberg unsure if this is the right implementation. It seems to evaluate void and zero individually as parameters (the test confirms it gives 2 errors). Not sure if looking for the specific string in sourceFile is brittle. Also.. I may have missed some cases?

JoshuaKGoldberg

💪 good step in the right direction, but I think it can be simplified.

JoshuaKGoldberg · 2019-01-07T11:54:55Z

src/noVoidZeroRule.ts

+const FAILURE_STRING: string = 'Replace void 0 with undefined';
+
+export class Rule extends Lint.Rules.AbstractRule {
+    public static metadata: ExtendedMetadata = {


It's not immediately clear why void 0 is undesirable (many folks won't know what it does), so I think this should include a rationale field. Maybe something around the lines of...

`void 0`, which resolves to `undefined`, can be confusing to newcomers. Exclusively use `undefined` to reduce ambiguity.

...and also in the README.md table.

Truth. 👍

JoshuaKGoldberg · 2019-01-07T11:56:25Z

src/noVoidZeroRule.ts

+    public static metadata: ExtendedMetadata = {
+        ruleName: 'no-void-zero',
+        type: 'maintainability',
+        description: 'Avoid using void 0, prefer undefined',


The README.md table's description is a little nicer:

Avoid using `void 0`; use `undefined` instead.

JoshuaKGoldberg · 2019-01-07T11:56:38Z

README.md

+        <code>no-void-zero</code>
+      </td>
+      <td>
+        Avoid using <code>void 0</code>; use undefined instead.


Nit: missing <code> wrapping around the undefined

JoshuaKGoldberg · 2019-01-07T11:58:27Z

src/tests/NoVoidZeroRuleTests.ts

+    it('should fail when function is given argument of void 0', (): void => {
+        const inputScript: string = `
+            const baz = (void 0) => {};
+            function (void 0) { }


This isn't valid JS: a void expression can't be in place of a function parameter.

🙃 😬 Ah meant to declare then use? Can't remember.

JoshuaKGoldberg · 2019-01-07T11:59:45Z

src/noVoidZeroRule.ts

+            tsutils.isArrowFunction(node) ||
+            tsutils.isFunctionDeclaration(node) ||
+            tsutils.isFunctionExpression(node)
+        ) {


I'm not following why this second if statement is necessary. Void expressions can't be function parameters, so I think you can remove this whole block and just have the first if statement checking for a void 0.

The first block should find any void 0 expression anyway, since this walker will recurse through all nodes.

Void expressions can't be function parameters gotcha -- thought I saw it used as so. Makes sense.

JoshuaKGoldberg · 2019-01-07T12:04:17Z

src/tests/NoVoidZeroRuleTests.ts

@@ -0,0 +1,62 @@
+import { Utils } from '../utils/Utils';


Would you mind switching this to the newer test format, actually? Under the test/ directory, copy any one that doesn't have sub-directories (which would be necessary if this rule had options) such as non-literal-fs-path, modify the tslint.json to only include no-void-0, and add your test cases there.

Tests in general are moving to that format (#489) because it's quite a bit nicer to read & write.

Sure! Good to know -- thank you!

JoshuaKGoldberg · 2019-01-07T12:07:09Z

src/noVoidZeroRule.ts

+
+export class Rule extends Lint.Rules.AbstractRule {
+    public static metadata: ExtendedMetadata = {
+        ruleName: 'no-void-zero',


`ruleName: 'no-void-zero',

IMO it's better to move away from prefixing rule names with no- for a couple of reasons:

It discourages rules from later being modified to support the opposite case. That's not so relevant to this rule, but does for others (e.g. parameter-reassignment; what if we want prefer reassigning them over making unnecessary new variables?)

It messes with sorting of rules, since some have no- and others don't

👍 Makes total sense

Do you think just void-zero (void-0 ?) is sufficient or prefer-undefined?

void-zero. names around preferring undefined could get confused with rules in the null vs. undefined area.

JoshuaKGoldberg · 2019-01-07T12:08:28Z

Re your edge cases - I can't think of too many others right now 😄 but it seems fine to go in with just a few. Maybe the different ways variables/properties can be assigned, such as parameters, variables, class members, and static class members?

lizzzp1 · 2019-01-09T02:41:19Z

Updated. The tests are an interesting format. Would there be no need need for a test runner if these were all this format or would you still need mocha/chai set up?

JoshuaKGoldberg

So close!

README.md

IllusionMH · 2019-01-09T10:06:46Z

Updated. The tests are an interesting format. Would there be no need need for a test runner if these were all this format or would you still need mocha/chai set up?

There are tests for utility methods that are used in rules implementation, so mocha/chai setup will be still used, but only those utility methods.

IllusionMH · 2019-01-09T12:52:42Z

This is pretty simple rule with concrete instruction what to do. May be it make sense to do add fixer?

@lizzzp1 would you prefer to add fixer in this PR or should I create separate issue for enhancement?

IllusionMH

Only two minor notes from me. Otherwise looks great 👍

IllusionMH · 2019-01-09T12:42:24Z

README.md

+      <td>
+        <code>void 0</code>, which resolves to <code>undefined</code>, can be confusing newcomers. Exclusively use <code>undefined</code> to reduce ambiguity.
+      </td>
+      <td>6.0.0</td>


6.0.0 is current verstion. This rule will be added in next minor version - 6.1.0.

Oh I thought you were linking to this when you said one more question. Define fixer :)

src/voidZeroRule.ts

lizzzp1 · 2019-01-09T13:03:37Z

@IllusionMH updated.

JoshuaKGoldberg

🙌 thanks @lizzzp1!

IllusionMH

That was fast! 🏎
Thank you!

IllusionMH · 2019-01-09T13:24:09Z

Only one question left above about fixer for this rule see above

Otherwise I think this PR is good to go unless @lizzzp1 would prefer implement fix in scope of current PR.

README.md

IllusionMH · 2019-01-09T13:46:28Z

I will post here instead of comment for line

Fixer is code that will fix lint error (in this case replace void 0 expression to just undefind) if user runs tslint with --fix option.
Example in this repo: https://github.com/Microsoft/tslint-microsoft-contrib/blob/59fa06e239f0a84da9d55113006ab0e8a6a7553a/src/importNameRule.ts#L175-L176
and 1 line in meta for consistency: https://github.com/Microsoft/tslint-microsoft-contrib/blob/59fa06e239f0a84da9d55113006ab0e8a6a7553a/src/importNameRule.ts#L14

Example in docs (no direct link to paragraph, example in the end): https://palantir.github.io/tslint/develop/custom-rules/

lizzzp1 · 2019-01-09T14:37:10Z

@IllusionMH 💡ah, yes the --fix flag, got it. I'm happy to do that here or if you think it's easier to do it in another issues, that's fine too. Would it be helpful for me to add this to the contributor read me? This might be common knowledge, but if you think it would be helpful, let me know.

IllusionMH · 2019-01-09T15:05:18Z

I'm happy to do that here or if you think it's easier to do it in another issues, that's fine too.

@lizzzp1 since fixer for this rule should be easy to implement, I'd prefer to have everything in this PR.

Please note that fixer behavior should be tested with adding tests/void-zero/test.ts.fix file that has all replacements in place.
You can check test.ts.lint and corresponding test.ts.fix for example (links to PR).

If you'll ahve any problems do not hesitate to ask here.

Would it be helpful for me to add this to the contributor read me? This might be common knowledge, but if you think it would be helpful, let me know.

I've created #779 to track this suggestion.

lizzzp1 · 2019-01-09T15:54:09Z

@IllusionMH sounds good, thanks!

IllusionMH · 2019-01-10T12:23:06Z

Unfortunately I forgot that #665 is not merged.
You'll need to add *.fix eol=lf to .gitattributes (see .gitattributes diff to avoid conflicts) and ensure that tests/void-zero/test.ts.fix has only LF line endings (should have correct already)

IllusionMH

@lizzzp1 Code, tests, and fixer looks great! Thank you for prompt responses and fixer implementation! 🙌

@JoshuaKGoldberg since only problem with line endings on Travis CI on Windows - I'm in favor of mergin this PR as is and adding line to .gitattributes in separate PR. What do you think?

JoshuaKGoldberg · 2019-01-10T18:17:39Z

Yeah SGTM! I can work on the other, bigger PRs in a few minutes.

* add rule and test * feeback - simplify rule, update to new test format, fix descriptions * feedback - fix typo, version, rationale on one line * feedback -add fix for rule * Remove extra line

add rule and test

3a42d94

lizzzp1 changed the title ~~add rule and test~~ no-void-rule Jan 7, 2019

lizzzp1 changed the title ~~no-void-rule~~ no-void-0-rule Jan 7, 2019

JoshuaKGoldberg approved these changes Jan 7, 2019

View reviewed changes

feeback - simplify rule, update to new test format, fix descriptions

98beaad

lizzzp1 changed the title ~~no-void-0-rule~~ void-0-rule Jan 9, 2019

JoshuaKGoldberg suggested changes Jan 9, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

IllusionMH suggested changes Jan 9, 2019

View reviewed changes

IllusionMH added the PR: Waiting for Author Changes have been requested that the pull request author should address. label Jan 9, 2019

feedback - fix typo, version, rationale on one line

0dd7805

JoshuaKGoldberg approved these changes Jan 9, 2019

View reviewed changes

IllusionMH approved these changes Jan 9, 2019

View reviewed changes

IllusionMH removed the PR: Waiting for Author Changes have been requested that the pull request author should address. label Jan 9, 2019

lizzzp1 commented Jan 9, 2019

View reviewed changes

README.md Show resolved Hide resolved

IllusionMH mentioned this pull request Jan 9, 2019

Add more info about writing new rules to Contribution guidlines #779

Closed

IllusionMH added the PR: Waiting for Author Changes have been requested that the pull request author should address. label Jan 9, 2019

lizzzp1 and others added 2 commits January 9, 2019 22:09

feedback -add fix for rule

8eb8c85

Remove extra line

b870487

IllusionMH added PR: Waiting for Author Changes have been requested that the pull request author should address. and removed PR: Waiting for Author Changes have been requested that the pull request author should address. labels Jan 10, 2019

JoshuaKGoldberg approved these changes Jan 10, 2019

View reviewed changes

IllusionMH approved these changes Jan 10, 2019

View reviewed changes

IllusionMH mentioned this pull request Jan 10, 2019

Enforce line endings for .fix files #782

Merged

2 tasks

IllusionMH merged commit 07567bf into microsoft:master Jan 10, 2019

IllusionMH added this to the 6.1.0-beta milestone Feb 19, 2019

void-0-rule #770

void-0-rule #770

Uh oh!

Conversation

lizzzp1 commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

Uh oh!

JoshuaKGoldberg left a comment

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!

lizzzp1 Jan 7, 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!

JoshuaKGoldberg Jan 7, 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!

lizzzp1 Jan 8, 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!

JoshuaKGoldberg commented Jan 7, 2019

Uh oh!

lizzzp1 commented Jan 9, 2019

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IllusionMH commented Jan 9, 2019

Uh oh!

IllusionMH commented Jan 9, 2019

Uh oh!

IllusionMH left a comment

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!

Uh oh!

lizzzp1 commented Jan 9, 2019

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

IllusionMH left a comment • 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!

IllusionMH commented Jan 9, 2019

Uh oh!

lizzzp1 commented Jan 7, 2019 •

edited

Loading

lizzzp1 Jan 7, 2019 •

edited

Loading

JoshuaKGoldberg Jan 7, 2019 •

edited

Loading

lizzzp1 Jan 8, 2019 •

edited

Loading

IllusionMH left a comment •

edited

Loading

lizzzp1 commented Jan 9, 2019 •

edited

Loading