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

update flake8 #33190

Closed
wants to merge 1 commit into from
Closed

update flake8 #33190

wants to merge 1 commit into from

Conversation

AmitPhulera
Copy link
Contributor

Product Description

We were couple of versions behind on the flake-8. Since it is a dev dependency and should be safe to upgrade, raising out this PR to do the same.

Safety Assurance

Dev dependency, safe to update. Just affects our linting process.

Automated test coverage

NA

QA Plan

NA

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@AmitPhulera AmitPhulera requested a review from a team as a code owner July 3, 2023 06:42
@dimagimon dimagimon added the dependencies Pull requests that update a dependency file label Jul 3, 2023
@millerdev
Copy link
Contributor

millerdev commented Jul 3, 2023

I think this breaks our pre-commit hook because it still uses the --diff parameter, which was deprecated in v5 and has been removed in v6.

git diff --cached -U0 | flake8 --diff --show-source --config=.flake8

As this issue points out, the --diff option was broken because it did not catch all issues introduced by changed lines.

One option would be to use use flake8-diff, although I'd be a little surprised if that does not have the same flaw as --diff did. Research needed.

Another option is to run flake8 against whole files and recommend that people fix the whole file when they make changes to it. If the changes are extensive, definitely do that in a separate commit, and possibly in a separate PR to reduce the volume of changes in otherwise risky PRs.

@AmitPhulera
Copy link
Contributor Author

Thanks for the info @millerdev . I now remember this being discussed before, I guess Graham brought it up. I will hold off this PR for now to explore other options.

@AmitPhulera AmitPhulera deleted the ap/update-flake8 branch August 14, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants