[new-rule]prefer-while rule #3750

rwaskiewicz · 2018-03-01T02:11:13Z

PR checklist

Addresses an existing issue: Move some rules of tslint-consistent-codestyle to tslint core #2040
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Enforces new lines after file header via new configuration key to be provided in TSLint file
Based off of https://github.com/ajafff/tslint-consistent-codestyle/blob/master/rules/preferWhileRule.ts

JoshuaKGoldberg · 2018-04-20T04:37:14Z

src/rules/preferWhileRule.ts

+        const failures: Lint.RuleFailure[] = [];
+
+        const cb = (node: ts.Node): void => {
+            if (node.kind === ts.SyntaxKind.ForStatement && this.doesNodeViolateRule(node as ts.ForStatement)) {


Use isForStatement from tsutils so you don't need the hardcast

TIL - nice! Updated in 7eda22c

JoshuaKGoldberg · 2018-04-20T04:37:15Z

src/rules/preferWhileRule.ts

+    };
+    /* tslint:enable:object-literal-sort-keys */
+
+    public static FAILURE_STRING = "prefer while";


Seems rather terse compared to other error messages?

Agreed, definitely something I overlooked. Updated to something a little more descriptive in 7eda22c:

Expected a 'while' loop instead of a 'for' loop without an initializer and incrementor

JoshuaKGoldberg · 2018-04-21T22:28:11Z

src/rules/preferWhileRule.ts

+        optionsDescription: "Not configurable.",
+        options: null,
+        optionExamples: [true],
+        hasFix: true,


Sorry to harp on this again 😊 but could you please add a rationale as well? It's not clear from just the description why such a thing is better.

No problem 🙂 Done in 84c6d8e

giladgray

@rwaskiewicz cool feature! some feedback that needs to be addressed...

giladgray · 2018-05-01T21:45:07Z

src/rules/preferWhileRule.ts

+    /* tslint:disable:object-literal-sort-keys */
+    public static metadata: Lint.IRuleMetadata = {
+        ruleName: "prefer-while",
+        description: "Prefer while loops when instead of a for loop without an initializer and incrementor.",


remove the words "when" and "a" and add backticks around while and for to distinguish them from english.

Done in ec8b337

giladgray · 2018-05-01T21:45:30Z

src/rules/preferWhileRule.ts

+    };
+    /* tslint:enable:object-literal-sort-keys */
+
+    public static FAILURE_STRING = "Expected a 'while' loop instead of a 'for' loop without an initializer and incrementor";


i'd just reuse the description above here, particularly the word "prefer" (like the rule name)

Done in ec8b337. I ended up combining tests in a separate commit, so there's a little bit of churn here with files that will end up getting combined (sorry about that!)

giladgray · 2018-05-01T21:49:55Z

test/rules/prefer-while/bad-with-condition/tslint.json

@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "prefer-while": true


tests that share the same config file should appear in the same .ts.lint file. please merge all of these so there's only true and false tests. one .ts.lint file can have more than one "test case" in it.

Done in 8fa8062. I added some comments to the test cases. Please let me know if they look amiss or should be added in the first place.

rwaskiewicz · 2018-05-02T00:43:25Z

@giladgray Thanks for the feedback! I've updated this branch as per your comments. CI seems to be failing on a non-related test, LMK if any action is needed on my part there or if you have any additional comments 🙂

suchanlee · 2018-05-02T08:14:33Z

@rwaskiewicz can you merge master into your branch or rebase off of it and see if the build still fails?

rwaskiewicz · 2018-05-02T11:08:12Z

@suchanlee I rebased off the latest master locally (f47aeb3). When rerunning npm run compile (locally) I ran into a different issue with completed-docs:

src/rules/completedDocsRule.ts(186,39): error TS1005: ']' expected.
src/rules/completedDocsRule.ts(186,86): error TS1128: Declaration or statement expected.
src/rules/completedDocsRule.ts(222,16): error TS1005: ';' expected.
src/rules/completedDocsRule.ts(224,18): error TS1005: ';' expected.
src/rules/completedDocsRule.ts(256,13): error TS1109: Expression expected.
src/rules/completedDocsRule.ts(256,14): error TS1128: Declaration or statement expected.
src/rules/completedDocsRule.ts(257,10): error TS1128: Declaration or statement expected.
src/rules/completedDocsRule.ts(283,18): error TS1005: ';' expected.
src/rules/completedDocsRule.ts(289,13): error TS1005: ';' expected.
src/rules/completedDocsRule.ts(290,23): error TS1005: ';' expected.
src/rules/completedDocsRule.ts(291,25): error TS1005: ';' expected.
src/rules/completedDocsRule.ts(292,5): error TS1109: Expression expected.
src/rules/completedDocsRule.ts(295,5): error TS1128: Declaration or statement expected.
src/rules/completedDocsRule.ts(295,13): error TS1128: Declaration or statement expected.
src/rules/completedDocsRule.ts(297,5): error TS1128: Declaration or statement expected.
src/rules/completedDocsRule.ts(297,39): error TS1005: ',' expected.
src/rules/completedDocsRule.ts(297,63): error TS1005: ',' expected.
src/rules/completedDocsRule.ts(297,76): error TS1005: ';' expected.
src/rules/completedDocsRule.ts(297,97): error TS1005: ';' expected.
src/rules/completedDocsRule.ts(304,5): error TS1128: Declaration or statement expected.
src/rules/completedDocsRule.ts(304,43): error TS1005: ',' expected.
src/rules/completedDocsRule.ts(304,88): error TS1005: '(' expected.
src/rules/completedDocsRule.ts(304,89): error TS1005: ',' expected.
src/rules/completedDocsRule.ts(304,105): error TS1005: ',' expected.
src/rules/completedDocsRule.ts(305,26): error TS1005: ',' expected.
src/rules/completedDocsRule.ts(305,34): error TS1005: ',' expected.
src/rules/completedDocsRule.ts(305,41): error TS1005: ';' expected.
src/rules/completedDocsRule.ts(310,5): error TS1128: Declaration or statement expected.
src/rules/completedDocsRule.ts(311,1): error TS1128: Declaration or statement expected.

Looking at the latest CI builds for the master branch, it looks like this is occurring there as well. Would you prefer I pushed the rebase branch now, or hold off for the moment?

suchanlee · 2018-05-02T16:33:38Z

Oh hmm ill look into the failing build. Please hold off for now

rwaskiewicz · 2018-05-02T22:43:50Z

@suchanlee I've tracked it down to an issue with missing backticks in the completed-docs rule. I've put up #3868 to address. Could you please take a look when you get a chance?

suchanlee · 2018-05-02T23:23:48Z

@rwaskiewicz thanks for getting to that so fast! I just merged it.

Implements prefer-while rule in place of a for loop without an initializer and incrementer

- Use isForStatement from tsutils - Update failure string to be more descriptive - Updated test messages

- assertion now uneccessary with using isForStatement from tsutils

- Updated the description and failure string of prefer-while - Updated tests accordingly

- Combine test cases for 'bad' for loops and case for 'good' for loops - Remove unneeded files - Add comments to test cases

rwaskiewicz · 2018-05-03T00:06:32Z

@suchanlee Awesome! Rebased off the latest master and CI appears to be passing now 🙂

suchanlee · 2018-05-03T00:57:41Z

Hey @rwaskiewicz one thing we recently added to our docs is code examples of what passes and fails. It would be awesome if something like that can be added for this new rule. Example: https://github.com/palantir/tslint/pull/3602/files#diff-25413efe179575004d7787097d959e36R1

suchanlee · 2018-05-03T01:02:16Z

test/rules/prefer-while/test.ts.fix

+// for loops with an initializer, termination condition, and incrementor should remain untouched
+for(let x = 1; x < 10; x++) {
+    console.log(x);
+}


can we add a test case for when it's incremented with x+=1?
does the rule handle the case when the increment happens within the block? Example:

for (let i = 0; i < 10;) { i += 1; }

Yes and yes! Added test cases in 187bc3d

- Added test case with incrmentor in body of loop - Added test case with += incrementor

- Added code examples for prefer-while rule

rwaskiewicz · 2018-05-03T01:44:29Z

@suchanlee Regarding code-examples, that looks super useful! I took a stab at them in 61d8259 - LMK what you think

suchanlee · 2018-05-03T06:01:59Z

Thank you for your contribution @rwaskiewicz!

rwaskiewicz mentioned this pull request Mar 1, 2018

Move some rules of tslint-consistent-codestyle to tslint core #2040

Closed

JoshuaKGoldberg reviewed Apr 20, 2018

View reviewed changes

JoshuaKGoldberg reviewed Apr 21, 2018

View reviewed changes

giladgray suggested changes May 1, 2018

View reviewed changes

rwaskiewicz mentioned this pull request May 2, 2018

[fix-build] Fix completed-docs rule #3868

Merged

4 tasks

rwaskiewicz added 9 commits May 2, 2018 20:01

prefer-while rule

45cfcea

Implements prefer-while rule in place of a for loop without an initializer and incrementer

renamed failure object creation method

5590243

fix json test file indentatons

40b71b0

Address review comments

0f83a6f

- Use isForStatement from tsutils - Update failure string to be more descriptive - Updated test messages

remove uneccessary type assertion

1b09f36

- assertion now uneccessary with using isForStatement from tsutils

Add rationale field to prefer-while rule metadata

569d0b8

Use 'let' instead of 'const' in test

b825e27

Update rule description & failure string

596f532

- Updated the description and failure string of prefer-while - Updated tests accordingly

Coalesce test cases

eebadbc

- Combine test cases for 'bad' for loops and case for 'good' for loops - Remove unneeded files - Add comments to test cases

rwaskiewicz force-pushed the prefer-while branch from 8fa8062 to eebadbc Compare May 3, 2018 00:03

suchanlee reviewed May 3, 2018

View reviewed changes

rwaskiewicz added 2 commits May 2, 2018 21:24

Add additional test cases

187bc3d

- Added test case with incrmentor in body of loop - Added test case with += incrementor

Add code examples

61d8259

- Added code examples for prefer-while rule

suchanlee approved these changes May 3, 2018

View reviewed changes

suchanlee merged commit baa413e into palantir:master May 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[new-rule]prefer-while rule #3750

[new-rule]prefer-while rule #3750

rwaskiewicz commented Mar 1, 2018 •

edited

Loading

JoshuaKGoldberg Apr 20, 2018

rwaskiewicz Apr 21, 2018

JoshuaKGoldberg Apr 20, 2018

rwaskiewicz Apr 21, 2018

JoshuaKGoldberg Apr 21, 2018

rwaskiewicz Apr 22, 2018

giladgray left a comment

giladgray May 1, 2018

rwaskiewicz May 2, 2018

giladgray May 1, 2018

rwaskiewicz May 2, 2018

giladgray May 1, 2018

rwaskiewicz May 2, 2018

rwaskiewicz commented May 2, 2018

suchanlee commented May 2, 2018

rwaskiewicz commented May 2, 2018

suchanlee commented May 2, 2018

rwaskiewicz commented May 2, 2018

suchanlee commented May 2, 2018

rwaskiewicz commented May 3, 2018

suchanlee commented May 3, 2018

suchanlee May 3, 2018

rwaskiewicz May 3, 2018

rwaskiewicz commented May 3, 2018

suchanlee commented May 3, 2018

[new-rule]prefer-while rule #3750

[new-rule]prefer-while rule #3750

Conversation

rwaskiewicz commented Mar 1, 2018 • edited Loading

PR checklist

Overview of change:

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

giladgray left a 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

Choose a reason for hiding this comment

rwaskiewicz commented May 2, 2018

suchanlee commented May 2, 2018

rwaskiewicz commented May 2, 2018

suchanlee commented May 2, 2018

rwaskiewicz commented May 2, 2018

suchanlee commented May 2, 2018

rwaskiewicz commented May 3, 2018

suchanlee commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwaskiewicz commented May 3, 2018

suchanlee commented May 3, 2018

rwaskiewicz commented Mar 1, 2018 •

edited

Loading