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

chore: Allow flake8 6.x #280

Closed
wants to merge 12 commits into from
Closed

Conversation

jashparekh
Copy link
Contributor

@jashparekh jashparekh commented Nov 23, 2022

flake8 6.0 was recently released: https://flake8.pycqa.org/en/latest/release-notes/6.0.0.html.
No breaking change should impact the plugin.

I've kept an upper bound to be consistent with previous changes, but would you consider not setting one? iscinumpy.dev/post/bound-version-constraints is an interesting read on the subject.

Should resolve #281

@sobolevn
Copy link
Member

Looks like we would need to drop 3.7 support for this to work 😒

@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #280 (b9a1eaa) into master (52c60e1) will not change coverage.
The diff coverage is n/a.

❗ Current head b9a1eaa differs from pull request most recent head 671a06e. Consider uploading reports for the commit 671a06e to get more accurate results

@@            Coverage Diff            @@
##            master      #280   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           22        22           
  Branches         4         4           
=========================================
  Hits            22        22           

@jashparekh
Copy link
Contributor Author

Looks like we would need to drop 3.7 support for this to work 😒

I can do that too

pyproject.toml Outdated Show resolved Hide resolved
@sobolevn
Copy link
Member

I really don't know what to do.

  1. I won't drop python3.7 in my projects until its EOL is reached
  2. I still want users to be able to use recent flake8 releases

@jashparekh
Copy link
Contributor Author

I really don't know what to do.

  1. I won't drop python3.7 in my projects until its EOL is reached
  2. I still want users to be able to use recent flake8 releases

I am not if there is a way out here as well, most of my apps are on 3.10 but I get the overhead that not everyone would be able to take on the overhead of migrating off of 3.7 just yet. I will read my up the release note closely later today. Not sure what was the reason for flake8 to drop support for 3.7 so soon.

@sobolevn
Copy link
Member

It is probably due to importlib-metadata problem

@jashparekh
Copy link
Contributor Author

Ya that was exactly why 3.8.1 was preferred. Currently we are blocked with flake8 upgrade due to this, I will hold off next week and let you make the final call here.

Happy Thanksgiving!

@jonyscathe
Copy link

Is there any issue with still allowing python 3.7?
That way those still on python 3.7 can use the latest flake8-broken-line with flake8==5.x.x and those on python >3.7 can start using flake8 6.

@sobolevn
Copy link
Member

That way those still on python 3.7 can use the latest flake8-broken-line with flake8==5.x.x and those on python >3.7 can start using flake8 6.

Sound like the best option for me.

@jashparekh
Copy link
Contributor Author

That way those still on python 3.7 can use the latest flake8-broken-line with flake8==5.x.x and those on python >3.7 can start using flake8 6.

Sound like the best option for me.

Updated my PR accordingly!

@sobolevn
Copy link
Member

Why do we need typed-ast?

@jashparekh
Copy link
Contributor Author

Why do we need typed-ast?

I could not figure the exact cause but if you look here, the tests for python3.7 started failing because it was missing

@sobolevn
Copy link
Member

Are you sure that it is a test-only dependency then?

@jashparekh
Copy link
Contributor Author

Are you sure that it is a test-only dependency then?

yes

@jashparekh
Copy link
Contributor Author

Hey @sobolevn - Is there something still blocking this PR? We are blocked across few projects with our Flake8 upgrade because of this

@bellini666
Copy link

If you can merge and do a new release, we are also blocked from updating flake8 until this gets released.

Also, I would suggest maybe removing the upper bound from the requirement so that this doesn't happen again in the future (maybe not in this PR to avoid taking more time to release it)

@jashparekh
Copy link
Contributor Author

If you can merge and do a new release, we are also blocked from updating flake8 until this gets released.

Also, I would suggest maybe removing the upper bound from the requirement so that this doesn't happen again in the future (maybe not in this PR to avoid taking more time to release it)

I like the idea of removing the upper bound but also agree that it should be a separate PR to do so. I can create the PR once this merged

@matt-carr
Copy link

Looks like we would need to drop 3.7 support for this to work 😒

I don't think this is true? Someone correct me if I'm crazy here, but:

  • because this is a plugin, we count on the consumer's dependency resolution to pick versions
  • There's nothing specifically in this library that is incompatible with py3.7
  • If we expand the version clamp to flake8<=6, all that means is that we're stating that it can use any flake8 version between 3.5 and 6

So, basically, if we expand the version clamp, it's up to the user to pick if they want to stick with py3.7 and flake8 5.0.4 or bump to py3.8.1+ and flake8 6.0.0, but in either case this should be able to plug into either one because it's not dependent on any specific functionality in flake8 6 or python 3.8.1.

Basically, this plugin will have no compatibility problems, and so you should be able to expand compatible versions to flake8 6

@jashparekh
Copy link
Contributor Author

Looks like we would need to drop 3.7 support for this to work 😒

I don't think this is true? Someone correct me if I'm crazy here, but:

  • because this is a plugin, we count on the consumer's dependency resolution to pick versions
  • There's nothing specifically in this library that is incompatible with py3.7
  • If we expand the version clamp to flake8<=6, all that means is that we're stating that it can use any flake8 version between 3.5 and 6

So, basically, if we expand the version clamp, it's up to the user to pick if they want to stick with py3.7 and flake8 5.0.4 or bump to py3.8.1+ and flake8 6.0.0, but in either case this should be able to plug into either one because it's not dependent on any specific functionality in flake8 6 or python 3.8.1.

Basically, this plugin will have no compatibility problems, and so you should be able to expand compatible versions to flake8 6

That is accurate, the current PR incorporates takes this into consideration and does not drop support for 3.7

@jashparekh
Copy link
Contributor Author

Hi @sobolevn - Following up on the review of this PR, since its blocking lot of people and projects

christophmeissner added a commit to coders4help/volunteer_planner that referenced this pull request Dec 5, 2022
Flake8 plugin removed because of dependency incompatibility. See #704. Once, this gets merged we may consider using it again: wemake-services/flake8-broken-line#280.
xatier added a commit to xatier/rc-files that referenced this pull request Dec 10, 2022
Copy link

@Dreamsorcerer Dreamsorcerer left a comment

Choose a reason for hiding this comment

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

As mentioned above, this looks correct. Py 3.7 users will still install flake8 v5, while others users can install v6.

I'd probably make it a patch release though (0.6.1) given no code or backwards-compatibility changes have taken place.

@jashparekh
Copy link
Contributor Author

As mentioned above, this looks correct. Py 3.7 users will still install flake8 v5, while others users can install v6.

I'd probably make it a patch release though (0.6.1) given no code or backwards-compatibility changes have taken place.

Done

@jashparekh
Copy link
Contributor Author

Hey @sobolevn - Reaching out one more time for a review on this PR

@decaz
Copy link

decaz commented Dec 28, 2022

@sobolevn are any updates on this?

@jashparekh
Copy link
Contributor Author

Hey @sobolevn - Reaching out for review again

@jashparekh
Copy link
Contributor Author

Hi @sobolevn - Following up again on this, can you share what's holding this PR to be approved

@chopeen
Copy link

chopeen commented Feb 21, 2023

Hello @sobolevn - can you please let us know if and when this PR can be accepted?

If you need to abandon this plugin altogether, that's also valuable information - then we can consider forking the project to publish a new package to PyPI or look for alternatives.

@jashparekh
Copy link
Contributor Author

Hi @sobolevn - I am planning to fork this project next week and publish a new package, please let us know soon if you plan to support this plugin

@chopeen
Copy link

chopeen commented Apr 3, 2023

FYI @jashparekh, I decided to switch my pipelines to Ruff https://beta.ruff.rs/docs/rules/.

@terencehonles
Copy link

Looks like this is superseded by #307

@jashparekh jashparekh closed this Jun 19, 2023
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.

Compat with flake8 >= 6
9 participants