General Jest padding rule #44

benkimpel · 2019-07-02T19:09:45Z

The code in rules/padding.ts is based on the ESLint rule
padding-line-between-statement. It's been altered to look for Jest
tokens in expressions.

Added rules and test for afterAll, afterEach, beforeAll, beforeEach,
describe, expect, it/test, and all tokens together.

Additional changes

Pointed package.json dev dependency for this package to itself and set up linking
Tweak to .gitignore to make lib absolute. (It was ignoring my new tests in test/lib.)

benkimpel · 2019-07-02T19:13:35Z

Working on CI

benkimpel · 2019-07-02T19:15:16Z

Haha... I mean I'm working on fixing CI.

benkimpel · 2019-07-02T20:33:33Z

Sorry if I'm spamming CircleCI failures. There's something strange going on. If I SSH into the CircleCI box with "Rebuild job with SSH" I'm able to run yarn lint successfully. It's weird.

benkimpel · 2019-07-02T20:46:37Z

Ohhhhhhhhhh. I think I got it. I think it has something to do with how Circle is running commands.
sh -c /home/circleci/repo/node_modules/.bin/eslint .

Nope, just needed to link it

benkimpel · 2019-07-03T04:28:23Z

A glorious green check mark. That's why we're programmers, amirite? For the check marks.

benkimpel · 2019-07-07T03:41:01Z

Ok, @dangreenisrael. I think this PR is feature complete now. I'll clean up the commit message and add comments on the code on Monday. There are a few items that I'll call your attention to in particular... just to make sure they're not missed in review since they're minor changes.

And all green on CI and I've run it against my own web app with good results.

dangreenisrael · 2019-07-07T18:23:06Z

@benkimpel Starting the CR now. Thanks so much for doing this, and for your patience with my delayed code reviews.

package.json

src/index.ts

src/rules/padding.ts

src/utils.ts

dangreenisrael

Everything looks really great. I'm excited for this to land.

dangreenisrael · 2019-07-07T19:06:33Z

Regarding all the request for types: I generally consider all of them as nice to haves. I only recently converted the project to typescript and a lot of the existing stuff didn't have types to begin with so I would be quite a hypocrite to insist that all of your (awesome) stuff is fully typed.

That said, if you could add them that would be awesome, but if you don't want for some of them - feel free to just note that in the comments and mark the comments as resolved

dangreenisrael · 2019-07-07T19:13:02Z

Once this is all ready to go, one of us can update the main README

benkimpel · 2019-07-07T20:38:13Z

@dangreenisrael Great feedback. I'm on it (starting tomorrow).

Good thinking on the README... I totally forgot.

Type requests are fine by me!

src/rules/padding.ts

package.json

benkimpel · 2019-07-12T03:11:04Z

All right. I think I'll have a final commit tomorrow for last bit of clean up and type info. Exciting. I'll re-request a review when ready.

benkimpel · 2019-07-16T01:49:24Z

Ready for another look @dangreenisrael!

benkimpel · 2019-07-16T03:14:26Z

All right. I think I'll have a final commit tomorrow for last bit of clean up and type info. Exciting. I'll re-request a review when ready.
-- Ben Kimpel (4 days ago)

"tommorow"... heh. 4-days-ago-Ben so optimistic... so naive.

Anyway, if this gets the thumbs up I'll squash and merge (if that's your preferred workflow) and make a nice commit message. Or you can! LMK.

package.json

dangreenisrael

If you wouldn't mind, it would be great to break this up into a few meaningful commits with descriptions of what is happening in the commit messages.

That said, I only have one comment and then this thing is good to go!

The goal here is to use the code currently under development to lint/format itself. Using the local path `file:.` in `package.json` is a little clunky in some scenarios. As I understand it npm and yarn handle local paths a little differently. Latest npm symlinks the path, while yarn copies it. Copying isn't ideal for development because you'll need to re-install for each build to use your changes. Clunkiness aside, though, it still seems like the appropriate way to at least declare a dev dependency on the local state. The nicer solution for letting us use the current plugin against itself during development is to link it to itself. `yarn link` uses a symlink so any changes we're making during development (code changes, builds, formatting, etc) are also what we're running against the package itself. Linking will replace the copied package from local path in `node_modules` with a symlink.

https://eslint.org/docs/6.0.0/developer-guide/working-with-plugins#peer-dependency

This padding rule is based on eslint/padding-line-between-statements. `src/rules/padding.ts` reconfigures the eslint rule to look for Jest tokens and add padding lines. There are several bits of functionality that were not needed from the original and they have been removed. The file is also converted to TypeScript to match this project's preferred language.

Introduces a `makeRule` function to take a set of padding configs, copy the generic rule, configure it with the padding configs, and return it as a new rule. `makeRule` is now used in `src/index.ts` to generate the rules from a config instead of defining rules in their own modules and importing them. Minor doc and spec changes, as well.

Add rules - afterAll - afterEach - beforeAll - beforeEach - expect Update README.md

benkimpel · 2019-07-19T04:50:39Z

All right, @dangreenisrael. Squished down to 5 solid commits with good explanations. Merge whenever you're ready.

Removed the version bump in package.json and will leave it up to you to PR that change.

dangreenisrael · 2019-07-19T19:56:52Z

@benkimpel LGTM!

Would you like the honours of hitting the big green button?

*Merge commit please - those are some nice commit messages!!!

benkimpel added the POC Proof of concept, not intended to be merged as is label Jul 2, 2019

benkimpel force-pushed the general-jest-rules branch 2 times, most recently from 50031ec to 1551598 Compare July 3, 2019 04:06

benkimpel changed the title ~~Proof of concept for general Jest padding rule~~ General Jest padding rule Jul 3, 2019

benkimpel removed the POC Proof of concept, not intended to be merged as is label Jul 3, 2019

benkimpel force-pushed the general-jest-rules branch from c22c364 to 901b364 Compare July 7, 2019 03:28