search

anymail / django-anymail

Django email backends and webhooks for Amazon SES, Brevo (Sendinblue), MailerSend, Mailgun, Mailjet, Postmark, Postal, Resend, SendGrid, SparkPost, Unisender Go and more
https://anymail.dev
BSD 3-Clause "New" or "Revised" License
1.7k stars 131 forks source link

Utilize pre-commit, black, isort and doc8 #285

Closed tim-schilling closed 1 year ago

tim-schilling commented 2 years ago

It will be easier to contribute if the style preferences of the project were enforced automatically. pre-commit is a tool that supports this. I've set it up to apply black, isort and doc8 to standardize the coding style.

Obviously, this is a large change and I don't have a ton of involvement with the project historically. However, these tools have made my life easier on other projects, especially those that I am reviewing pull requests for.

Edit: If this is too much to take on, that's understandable and I won't be offended.

medmunds commented 2 years ago

Thanks for this. Black and isort have actually been on my "would be nice" list for a while, but never bubbled up to the top.

Probably the tox testenv:lint should also be updated to match. Or ideally just run the pre-commit hooks from tox -e lint (or vice versa) so that tox and pre-commit stay in sync.

I'd also like to use .git-blame-ignore-revs to make it easier to spot code changes around the mass reformatting. (So probably we'd want to split the reformatting into one rev that can be git-blame-ignored, and then add the pre-commit hooks in the next rev, which wouldn't be ignored. I think.)

tim-schilling commented 2 years ago

@medmunds what do you think about dropping support for Django 2-3.1 and Python 3.5? I realized that I configured black to target 3.8 rather than the lowest possible version.

tim-schilling commented 2 years ago

@medmunds I pulled out the style changes into a large commit and added that to .git-blame-ignore-revs. I've also removed the lint usage in tox in favor of setting up an integration with https://pre-commit.ci/ which is free for open source libraries. The CI portion of the integration will validate open PRs and enforce style guidelines.

medmunds commented 2 years ago

Thanks again for this, and sorry for the delayed responses—I'm kind of swamped with some other matters lately.

I'll drop a handful of specific questions/comments into a code review.

what do you think about dropping support for Django 2-3.1 and Python 3.5?

We can and should drop support for Django 2.x and Python 3.5. That pending deprecation was announced with Anymail v8.6 on 2022-05-15.

I'd like to keep Django 3.0 and 3.1 support (on Python 3.6 or later) until it's time to drop support for all Django 3.x. (As a general rule, I try to retain support for Django versions a while after Django ends support, so that Anymail is never the package forcing a project to upgrade.)

[Dropping version support should be a separate PR. PR welcome, or I'll try to get to that this weekend.]

removed the lint usage in tox

There are two goals for tox usage in this project: 1) If a developer runs tox locally, and the tests pass, they should be confident that their changes will also pass CI (with the possible exception of live integration tests). 2) Reduce or eliminate duplication of CI configuration. (We used to have a separate .travis-ci and tox.ini which were almost never in sync.)

So I'd like to keep the tox lint environment, and have it run the pre-commit hooks. I haven't worked with pre-commit before, but it seems like that would just be pre-commit run --all-files? (The pre-commit docs also have notes on usage with tox.)

As far as pre-commit.ci, I'd prefer to keep the CI all in GitHub Actions for now, just to minimize the number of service integrations that have to be maintained.

medmunds commented 2 years ago

(I've dropped Python 3.5 and Django 2.x support on the main branch, so black can target Python 3.6.)

tim-schilling commented 2 years ago

@medmunds thank you for the quality review. I haven't forgotten about this. My OSS availability is weaning a bit, but I'll try to address this over the weekend.

medmunds commented 2 years ago

@tim-schilling I completely understand—no rush.

medmunds commented 1 year ago

@tim-schilling sorry for the lengthy delay getting back to this. I've "rebased" to the latest main, and made a few adjustments and additions in the formatting config.

Decided to locate all the tool config in pyproject.toml rather than setup.cfg, in preparation for modernizing Anymail's packaging at some point in the (hopefully near) future. Exception is flake8, which doesn't read pyproject.toml—so just left that in .flake8.

Thanks again for doing the legwork on this.

tim-schilling commented 1 year ago

Thank you for maintaining the package and seeing this one through!