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

ref(py3): pre-commit upgrade + sweeping rerun + __future__ removals #23197

Merged
merged 18 commits into from Jan 25, 2021

Conversation

joshuarli
Copy link
Member

@joshuarli joshuarli commented Jan 20, 2021

For anyone rebasing/merging against this when it lands, you'll need to make setup-git and run pre-commit on the files you changed in your branch. Something like pre-commit run --files $(git whatchanged --name-only --pretty="" master... | sort | uniq).

This upgrades pre-commit and its hooks, moves it into a requirements-pre-commit.txt for git hook notifications on updates, and reruns pre-commit on everything. Most notably, black's target-version py36 removes u prefixes. flake8 is now run on bin/ and I manually made those fixes. Also removes all __future__s (in our case we can remove everything).

@joshuarli joshuarli requested a review from a team as a code owner January 20, 2021 19:54
@joshuarli joshuarli requested review from a team January 20, 2021 19:54
@joshuarli joshuarli requested review from a team as code owners January 20, 2021 19:54
@joshuarli joshuarli requested review from a team and removed request for a team January 20, 2021 19:54
@joshuarli
Copy link
Member Author

Sorry for the notifications, codeowners. lol

image

Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

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

unsubscribe

bin/dump-command-help Outdated Show resolved Hide resolved
@billyvg
Copy link
Member

billyvg commented Jan 20, 2021

Do we need to do this for getsentry as well?

@joshuarli
Copy link
Member Author

I'm probably going to wait for #23195. @wedamija would also like your review on https://github.com/getsentry/getsentry/pull/4957.

@joshuarli joshuarli changed the title ref(py3): pre-commit upgrade + sweeping rerun ref(py3): pre-commit upgrade + sweeping rerun + __future__ removals Jan 23, 2021
@joshuarli
Copy link
Member Author

Oh, might as well just remove all the __future__s. All the ones we have are not necessary in Python 3.6, you can refer to Python's Lib/__future__.py.

@joshuarli
Copy link
Member Author

joshuarli commented Jan 23, 2021

git fetch origin
git merge -Xtheirs origin/master
manually remove any files marked as UD (deleted upstream)
...let pre-commit run on the merge commit, or if there wasn't a pause,
pre-commit run --files $(git diff --name-only HEAD~1 HEAD)

@joshuarli
Copy link
Member Author

Ah backend lint cancelled at 3 min cap... @billyvg FYI this did not result in a "failure", I had to notice this.

image

Bumping to 10 for now.

@billyvg
Copy link
Member

billyvg commented Jan 23, 2021

Ah backend lint cancelled at 3 min cap... @billyvg FYI this did not result in a "failure", I had to notice this.

image

Bumping to 10 for now.

Yeah cancelled jobs have a subtle gray icon but will count as a failure if the check is required (which can't happen on gs)

@joshuarli
Copy link
Member Author

joshuarli commented Jan 23, 2021

lol

reformatted /Users/joshua.li/dev/sentry/sentry/src/sentry/migrations/0150_remove_userreport_eventattachment_constraints.py

examples/oauth2_consumer_webserver/app.py:1:1: B317: Use ``from sentry.utils import json`` instead.

but other than this we lookin' good, gonna wait until monday, remerge, and do all of this for https://github.com/getsentry/getsentry/pull/4957

(cherry picked from commit eff5835)
@joshuarli
Copy link
Member Author

Uhh, lol, guess I went at it too hard today.

Fetching list of changed files for PR#23197 from Github API
Error: API rate limit exceeded for installation ID 1204373.

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

Successfully merging this pull request may close these issues.

None yet

5 participants