dangreenisrael / eslint-plugin-jest-formatting

ESLint rules for formatting test suites written for jest.
MIT License
156 stars 13 forks source link

Allow `await`ed `expect`s to be next to regular ones #100

Closed tmercswims closed 3 years ago

tmercswims commented 3 years ago

Full disclosure: This is my first foray into the world of ESTrees, so this solution comes largely from a place of "well, I found a way to do it that works" and less one of "I know what I need to do and I did it."

So that said, if there is a completely different approach that you had in mind when first reading the issue I filed, then I'd be happy to re-approach this, or hand it off; whatever you prefer.

Fixes #98.

dangreenisrael commented 3 years ago

Thanks for the quick PR 🙏

benkimpel commented 3 years ago

@tmercswims @dangreenisrael This is great! Mind if I take a more careful look tomorrow during my free time?

tmercswims commented 3 years ago

Fine with me of course, take your time! 🙂

benkimpel commented 3 years ago

Absolutely makes sense, and something I probably should have thought to refine :sweat_smile: I've done the change pretty much exactly how you describe, and it works great.

@tmercswims Awesome! Glad it worked out. Thanks for the bug report and fix.

@dangreenisrael I approved PR, but I've got two questions for you related to merge: 1) How do you want to handle versioning on this? It's a bug fix for us, but it could break CI runs for our users w/o any code changes on their part. Not sure of the standard versioning approach for a linter, honestly. 2) Shall we squash these commits down to one?

dangreenisrael commented 3 years ago

1) Excellent point. On the one hand, it is a bug fix. On the other hand it is an incompatible change, this will require a code change for most users.

I tend to think that that would actually make it a major change, even though it is a bug fix. If you guys feel weird about that, another option would be to add this optional and opt in - but I think I am leaning towards major version change.

2) Squash and merge please. I am going to disable non-linear git history in the settings.

tmercswims commented 3 years ago

In my opinion a major version bump is the right way to go, since this definitely does have the potential to start calling out things that it didn't before. Putting it behind an option feels strange, since it seems like the sort of thing that should "just work."

Just my two cents.

dangreenisrael commented 3 years ago

@benkimpel any thoughts?

benkimpel commented 3 years ago

Major bump makes sense to me, too.

dangreenisrael commented 3 years ago

Sounds good. @tmercswims do you mind bumping the version in the package.json and updating the CHANGELOG?

benkimpel commented 3 years ago

Sounds good. @tmercswims do you mind bumping the version in the package.json and updating the CHANGELOG?

@tmercswims If it's not to late would you mind also squashing down to one commit with a commit message in roughly this format. We don't use conventional commit on this project, although that's possible some day.

tmercswims commented 3 years ago

Done and done. I went ahead and dated the version May 4th, in anticipation of it happening today, but I can always change it if it doesn't. I can also adjust the actual change note if you guys had something else in mind.

I also wasn't sure if more description was really necessary in the commit as it's pretty self-explanatory with just the subject, so I settled on nothing - if you would like me to add more detail there, I'd be happy to.

benkimpel commented 3 years ago

@tmercswims thanks