Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Freshen up support tooling. #222

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

Freshen up support tooling. #222

wants to merge 11 commits into from

Conversation

Flameeyes
Copy link
Contributor

@Flameeyes Flameeyes commented Apr 13, 2020

This includes a couple of fixes for 308 redirects, some extra debug logging, and a whole lot of tooling changes to use isort, black and pre-commit.

@Flameeyes Flameeyes force-pushed the master branch 4 times, most recently from 6b45d11 to 267b8f9 Compare April 14, 2020 10:48
@Flameeyes
Copy link
Contributor Author

So to come back from what I said on Twitter:

  • this is the set of changes that is safe and does not cover the whole half-rewrite I did, for that I want to mull it over a little more and come back with reviewable changes;
  • pip uses very few interfaces of cachecontrol, and that's good, it means that compatibility is not hard to maintain even while removing some of the features I removed to reduce complexity;
  • pip does have a modified FileCache implementation that might make sense to backport here, I'll take a look license compatibility later;
  • Python 2.7 support will be needed for pip compatibility, so I'll try to restore that in my branch as well; will probably use six to make it easier to convert this back to Python 3 only if possible.

What is your opinion on isort, black, mypy and pre-commit ? I can probably add all of those in on this pull request.

@Flameeyes Flameeyes changed the title Improving caching to be usable with feed fetchers. Freshen up support tooling. Apr 17, 2020
@ionrock
Copy link
Contributor

ionrock commented Apr 20, 2020

I think these changes are looking good to me. It would be nice to break things up a bit. For example, adding all the SPDX, switching to six, etc. in different PRs. So far I didn't see anything concerning, but if there was an issue, at least we could chat about it independently.

I'm definitely a fan of black and started using it here a while back, that said, I don't think I automated the process and had been simply relying on my editor to do the needful. I'm also a fan of mypy, but again, just haven't had a chance to use it. Using isort also seems nice, although I'm a little concerned I could get annoyed with it if it doesn't work well all the time. Coming from Go, not having to reorder imports is definitely nice.

As for pre-commit, I'm a little more hesitant. I'm not a huge fan of magic automation and that feels a little magical to me. I'll be honest though and mention I'm saying this from ignorance so I'm happy to give it a try and learn something new!

In any case, if you want to submit PRs to use this stuff, let's do it individually so it is easier to understand and chat about.

Thanks!

@Flameeyes
Copy link
Contributor Author

SG! Splitting up those that don't have obvious dependencies between each other.

The pre-commit "magic" is not really more magic than integrating with the editor — it basically just makes sure that black/isort are run before you complete the commit, but we can discuss that on the dedicated pull request.

I'll try to send them as independently as I can.

@Flameeyes Flameeyes marked this pull request as draft April 20, 2020 16:17
@Flameeyes
Copy link
Contributor Author

Sent a few changes through already, I would prefer getting those merged before I rebase the black/isort parts because there'd be a lot of conflicts on both otherwise :(

This makes it easier to figure out _why_ something fails to look up altogether.
When caching permanent redirects, if `body` is left to `None`, there's an infinite recursion that will lead to the caching to silently fail and not cache anything at all.

So instead, make `body` a required parameter, which can be empty (`''`) for cached redirects.
This is to workaround an isort bug that appears fixed in master, where the Transfer-Encoding: chunked line is interpreted as an encoding for the file.
This includes isort, black and some basic hygiene on text files.
This is a fairly common unix extension for backup files used at least by Emacsen and vim.
This makes it easier to use constants for status codes as well.
This is just a matter of cleanup, I can't think of any good reason for this
_not_ to be marked abstract.
This is a bit more nuanced in Python 3, where only EEXIST errors are
suppressed, to match the `delete` codepath.
- id: isort
additional_dependencies:
- toml
- repo: https://github.com/python/black
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants