syrusakbary / snapshottest

Snapshot Testing utils for Python 📸
MIT License
525 stars 102 forks source link

Fix developer tooling for black, flake8 #135

Closed medmunds closed 3 years ago

medmunds commented 3 years ago

Several minor fixes and improvements related to addition of black code formatter:

Makefile:

README:

Travis.yml:

tox.ini:

setup.cfg:

medmunds commented 3 years ago

(I couldn't tell whether you had intended to keep flake8 alongside black, but guessed "yes" based on flake8 still appearing in the make lint target and an accidentally-disabled Travis job.)

paulmelnikow commented 3 years ago
  • Install black in make install, so it is available for make format

  • Restore flake8 to make install, so it is available for make lint

Thanks for addressing this in particular. It wasn't possible until #133 was merged and was on my mind to address.

paulmelnikow commented 3 years ago

(I couldn't tell whether you had intended to keep flake8 alongside black, but guessed "yes" based on flake8 still appearing in the make lint target and an accidentally-disabled Travis job.)

Yes, definitely. 😁 They do complementary things.

I usually ignore E203, E302, W503, I201 in the flake8 config to avoid conflicts with black. Not sure if those should be added here at this time.

medmunds commented 3 years ago

Oh, OK, I see a problem. Travis is using make install—but with this PR that would include the developer tooling in the tests, which we don't really want. (Plus it fails on Python 3.5, because black is no longer available there.)

Two options:

Do you have a preference?

medmunds commented 3 years ago

I usually ignore E203, E302, W503, I201 in the flake8 config to avoid conflicts with black. Not sure if those should be added here at this time.

I just copied the (current) recommended flake8 settings in the black docs. Easy enough to add the others if it would be helpful.

paulmelnikow commented 3 years ago
  • Put make install back the way it was, and add a new make develop target that also installs black and flake8 (and update the README Contributing section)

This seems like a good approach to me.

paulmelnikow commented 3 years ago

I just copied the (current) recommended flake8 settings in the black docs. Easy enough to add the others if it would be helpful.

I wonder if W503 is no longer a conflict.

I think E302 and I201 could be left off, too. Those are just my own style preferences.

medmunds commented 3 years ago

Sounds like W503 is now disabled by default in flake8, since a PEP 8 change a while back.

I think I201 depends on flake8-import-order, which we aren't currently using. (I don't think that's included with flake8 by default.)

I have no opinion on E302. (Well, actually, I agree with you, but in the spirit of deferring to opinionated formatting tools, I'm keeping my own opinion to myself :grin:)

paulmelnikow commented 3 years ago

Good to know that I can remove W503 from other projects. Yea, let's add flake8-import-order at some point, and then decide how we feel about all the newlines. I feel like without E302 the imports take up a whole screenful. 😁

paulmelnikow commented 3 years ago

Ready to merge, right?

medmunds commented 3 years ago

Yep!

paulmelnikow commented 3 years ago

Thanks so much! 😁 🚀

medmunds commented 3 years ago

@paulmelnikow my pleasure. Thank you for jumping in as an active maintainer!