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

flake8 >4 support #132

Open
gitpushdashf opened this issue Aug 5, 2022 · 24 comments
Open

flake8 >4 support #132

gitpushdashf opened this issue Aug 5, 2022 · 24 comments

Comments

@gitpushdashf
Copy link

Hi!

It looks like this supports flake8 v4, but not v5? What would it take to bring it up to flake8 v5?

Thank you!

@pwoolvett
Copy link

Indeed. It does not look to be a quick patch though... 🤷

  • lots of changes in option parsing
  • lots of plugins now wont run because of the new regex code validation. Not that it affects us directly, but it wouldn't make sense to upgrade to flake8v5 if by doing so users would have to drop checkers because of this. Maybe we could support both versions for a while, but still would be quite a challenge.

If someone wants to tackle this, I guess we could have a more thorough conversation about it.

To answer your question: It would take a considerable refactor.

@berzi
Copy link

berzi commented Aug 22, 2022

Can you (or someone) define a more specific list of things that would need to be changed? I think supporting newer linter versions is essential and must come at some point, but I had such a good experience with Flakeheaven that I wouldn't like to stop using it in exchange for being able to update the linters.

@blablatdinov
Copy link

May be we can define list of microtasks (issues), which help make migrate to flake8 v5 more simple?

For example WPS creates milestone for integrate python3.8 support in their linter

@pwoolvett
Copy link

pwoolvett commented Oct 18, 2022

I like this idea.

We would need to refactor a bunch of stuff, and depending on the specific "patch", it would not be straightforward to support both versions as flakeheaven uses flake8 internal api which changes inadvertently. In those cases i guess we'd have to use a facade or similar to support both. This would also facilitate future migrations and contributions.

So the general idea would be

flakeheaven setup-> select patch implementation from library -> patch flake8 once by replacing (runtime) flake8's Application-> run requested commmand `

So the initial plan I guess would look like

  • install the new flake8 and write down conflicting points. This would yield an initial list of issues.
  • refactor to support both versions (this has to be broken down)
  • Test against both versions

@mcarans
Copy link

mcarans commented Oct 26, 2022

For steps 2 and 3, if it's going to be painful to support both versions, then perhaps there should be a clean break with flakeheaven 3.x supporting flake8 4 and flakeheaven 4.x supporting flake8 5? On the other hand, a facade might be useful if flake8 continues to change in breaking ways.

@berzi
Copy link

berzi commented Nov 8, 2022

I agree with @mcarans that supporting both versions could be hard, and it would also likely delay support for flake8 5 significantly.

What about releasing a breaking version of flakeheaven which supports flake8 5 without necessarily retaining support for earlier versions? It would give a quicker option to those who want to use flake8 5 (the others can just not upgrade), and also provide a way to explore what compatibility issues are actually there. Then a further version can be developed that supports the newest and older versions, if possible.

@lyz-code
Copy link

I also agree with @mcarans. I'd do as soon as possible a 4.0.0rc1 so that people can start using it and raise issues to be fixed.

The idea of the facade makes a lot of sense too. Has someone started working in this direction? The pin of flake8 < 5 is killing me, so I'd be willing to spend some time on this

@pwoolvett
Copy link

@lyz-code sounds great!

I had an initial approach feat/flake8v5 😳 , but havent had the time to complete it. Maybe we could setup a chat/talk (discord?) and list what would be required.

@lyz-code
Copy link

I'll be here for the next 30 mins if you can join that would be great

@pwoolvett
Copy link

pwoolvett commented Nov 25, 2022

Pushed a commit to the feat/flake8v5 - see ddee34a (just commented out some imports to have a working start point)

changes in the branch not strictly related to flake8v5:

Current MO so far:

  • define a compat package:
    • a base module where the interface and skeleton are defined, as well as performing implementation selection
    • a v4 module containing an implementation for flake8v4
    • a v5 module containing an implementation for flake8v5
  • create two venvs, both with fh installed in editable mode, one with flake8v4 and the other with flake8v5
  • run flakeheaven lint in both venvs -> see where it crashes -> add the common part to an abstractmethod in the interface -> implement the abstractmethod for v4 and v5

I left it at make_file_checker_manager, so contributors will probably have to work from there

So far, all commands except the lint have been ported (not tested thoroughly).

@lyz-code
Copy link

Thanks @pwoolvett for the detailed description, it will make it much easier to contribute <3

lyz-code added a commit to lyz-code/blue-book that referenced this issue Nov 25, 2022
feat(click#File System Isolation): File System Isolation

For basic command line tools with file system operations, the
`CliRunner.isolated_filesystem()` method is useful for setting the current
working directory to a new, empty folder.

```python
from click.testing import CliRunner
from cat import cat

def test_cat():
    runner = CliRunner()
    with runner.isolated_filesystem():
        with open("hello.txt", "w") as f:
            f.write("Hello World!")

        result = runner.invoke(cat, ["hello.txt"])
        assert result.exit_code == 0
        assert result.output == "Hello World!\n"
```

Pass `temp_dir` to control where the temporary directory is created. The
directory will not be removed by Click in this case. This is useful to integrate
with a framework like Pytest that manages temporary files.

```python
def test_keep_dir(tmp_path):
    runner = CliRunner()

    with runner.isolated_filesystem(temp_dir=tmp_path) as td:
        ...
```

feat(python_snippets#Pathlib make parent directories if they don't exist): Pathlib make parent directories if they don't exist

```python
pathlib.Path("/tmp/sub1/sub2").mkdir(parents=True, exist_ok=True)
```

From the
[docs](https://docs.python.org/3/library/pathlib.html#pathlib.Path.mkdir):

- If `parents` is `true`, any missing parents of this path are created as
  needed; they are created with the default permissions without taking mode into
  account (mimicking the POSIX `mkdir -p` command).

- If `parents` is `false` (the default), a missing parent raises
  `FileNotFoundError`.

- If `exist_ok` is `false` (the default), `FileExistsError` is raised if the
  target directory already exists.

- If `exist_ok` is `true`, `FileExistsError` exceptions will be ignored (same
  behavior as the POSIX `mkdir -p` command), but only if the last path component
  is not an existing non-directory file.

feat(python_snippets#Pathlib touch a file): Pathlib touch a file

Create a file at this given path.

```python
pathlib.Path("/tmp/file.txt").touch(exist_ok=True)
```

If the file already exists, the function succeeds if `exist_ok` is `true` (and
its modification time is updated to the current time), otherwise
`FileExistsError` is raised.

If the parent directory doesn't exist you need to create it first.

```python
global_conf_path = xdg_home / "autoimport" / "config.toml"
global_conf_path.parent.mkdir(parents=True)
global_conf_path.touch(exist_ok=True)
```

fix(pdm#Solve circular dependencies): Solve circular dependencies by manual constraining

It also helps to run `pdm update` with the `-v` flag, that way you see which are
the candidates that are rejected, and you can put the constrain you want. For
example, I was seeing the next traceback:

```
pdm.termui: Conflicts detected:
  pyflakes>=3.0.0 (from <Candidate autoflake 2.0.0 from https://pypi.org/simple/autoflake/>)
  pyflakes<2.5.0,>=2.4.0 (from <Candidate flake8 4.0.1 from unknown>)
```

So I added a new dependency to pin it:

```
[tool.pdm.dev-dependencies]
dependencies = [
    # Until flakeheaven supports flake8 5.x
    # flakeheaven/flakeheaven#132
    "flake8>=4.0.1,<5.0.0",
    "pyflakes<2.5.0",
]
```

If none of the above works, you can override them:

```
[tool.pdm.overrides]
"importlib-metadata" = ">=3.10"
```
@mcarans
Copy link

mcarans commented Dec 28, 2022

Maybe this issue should be renamed flake8 > 4 support since there is already flake8 v6.

@pwoolvett pwoolvett changed the title flake8 v5 support flake8 >4 support Dec 29, 2022
@fasavard
Copy link

fasavard commented Feb 8, 2023

This would be a great upgrade indeed. Autopep8 cannot be installed to its latest version with poetry since the dependencies cannot be resolved on pycodestyle
Because no versions of flakeheaven match >3.2.1,<4.0.0 and flakeheaven (3.2.1) depends on flake8 (>=4.0.1,<5.0.0), flakeheaven (>=3.2.1,<4.0.0) requires flake8 (>=4.0.1,<5.0.0). And because no versions of flake8 match >4.0.1,<5.0.0 and flake8 (4.0.1) depends on pycodestyle (>=2.8.0,<2.9.0), flakeheaven (>=3.2.1,<4.0.0) requires pycodestyle (>=2.8.0,<2.9.0). And because autopep8 (2.0.1) depends on pycodestyle (>=2.10.0) and no versions of autopep8 match >2.0.1,<3.0.0, flakeheaven (>=3.2.1,<4.0.0) is incompatible with autopep8 (>=2.0.1,<3.0.0). So, because shotgunevents depends on both autopep8 (^2.0.1) and flakeheaven (^3.2.1), version solving failed.

@snmishra
Copy link

We would need to refactor a bunch of stuff, and depending on the specific "patch", it would not be straightforward to support both versions as flakeheaven uses flake8 internal api which changes inadvertently. In those cases i guess we'd have to use a facade or similar to support both. This would also facilitate future migrations and contributions.

@pwoolvett I think supporting both flake8 versions is a bit too much work for only a temporary gain. I believe a breaking change is acceptable in this case, and people stuck on flake8 v4 can use older versions of flakeheaven. As it is right now, flakeheaven is becoming unusable as other tools have started moving to flake8 v5 or v6.

Just my thoughts, but I do not understand the implementation details enough.

@hackengineer
Copy link

Just an FYI, we started moving into heaven but it appears that we have to go back to hell until we can bring flake8 with us.
depends on both flake8 (^6.0.0) and flakeheaven (^3.2.1), version solving failed.

We appreciate the work yall.

aucampia added a commit to aucampia/rdflib that referenced this issue Apr 11, 2023
Replace bare `except:` with `except Exception`, there are some cases where it
can be narrowed further, but this is already an improvement over the current
situation.

This is somewhat pursuant to eliminating
[flakeheaven](https://github.com/flakeheaven/flakeheaven), as it no longer
supports the latest version of flake8
[[ref](flakeheaven/flakeheaven#132)]. But it also is
just the right thing to do as bare exceptions can cause problems.
aucampia added a commit to RDFLib/rdflib that referenced this issue Apr 12, 2023
Replace bare `except:` with `except Exception`, there are some cases where it
can be narrowed further, but this is already an improvement over the current
situation.

This is somewhat pursuant to eliminating
[flakeheaven](https://github.com/flakeheaven/flakeheaven), as it no longer
supports the latest version of flake8
[[ref](flakeheaven/flakeheaven#132)]. But it also is
just the right thing to do as bare exceptions can cause problems.
aucampia added a commit to aucampia/rdflib that referenced this issue Apr 12, 2023
Eliminate some occurrences of the following flake8 errors in tests:

* E265 block comment should start with '# '
* E266 too many leading '#' for block comment
* E402 module level import not at top of file
* E712 comparison to False should be 'if cond is False:' or 'if not cond:'
* E712 comparison to True should be 'if cond is True:' or 'if cond:'
* E722 do not use bare 'except'
* F401 ... imported but unused
* F403 ... used; unable to detect undefined names
* F405 ... may be undefined, or defined from star imports: ...
* F541 f-string is missing placeholders
* F841 local variable 'result' is assigned to but never used
* N806 variable 'TEST_DIR' in function should be lowercase

This is pursuant to eliminating
[flakeheaven](https://github.com/flakeheaven/flakeheaven), as it no longer
supports the latest version of flake8
[[ref](flakeheaven/flakeheaven#132)].
aucampia added a commit to RDFLib/rdflib that referenced this issue Apr 14, 2023
Eliminate some occurrences of the following flake8 errors in tests:

* E265 block comment should start with '# '
* E266 too many leading '#' for block comment
* E402 module level import not at top of file
* E712 comparison to False should be 'if cond is False:' or 'if not cond:'
* E712 comparison to True should be 'if cond is True:' or 'if cond:'
* E722 do not use bare 'except'
* F401 ... imported but unused
* F403 ... used; unable to detect undefined names
* F405 ... may be undefined, or defined from star imports: ...
* F541 f-string is missing placeholders
* F841 local variable 'result' is assigned to but never used
* N806 variable 'TEST_DIR' in function should be lowercase

This is pursuant to eliminating
[flakeheaven](https://github.com/flakeheaven/flakeheaven), as it no longer
supports the latest version of flake8
[[ref](flakeheaven/flakeheaven#132)].
@Pixel-Minions
Copy link

Hey guys! Thank you for the hard work, is there any eta for this?

@mcarans
Copy link

mcarans commented Jun 27, 2023

See this discussion. flakeheaven needs new maintainers although personally I am not using it any more and am using ruff now.

In case it's of any use for those wanting to migrate, this is my team's setup with Black, ruff etc. being called from Hatch with configuration in a .config folder:
https://github.com/OCHA-DAP/hdx-python-country

@Pixel-Minions
Copy link

Thank you @mcarans, I wonder if you use with your current setup legacy code base support, like the baseline

@mcarans
Copy link

mcarans commented Jun 27, 2023

@Pixel-Minions Prior to Hatch and ruff with a .config folder for tools configuration, I was using tox, setup.cfg and flake8-pyproject with a monolithic pyproject.toml as here:

https://github.com/OCHA-DAP/hdx-python-country/blob/517e87ad2691a45b2bfe7e3207e32774a780c7b1/pyproject.toml

Before I switched from flakeheaven to flake8-pyproject, it looked like this:
https://github.com/OCHA-DAP/hdx-python-country/blob/a673b28f0d1a3bdea3c2c3f265818d91b0c82572/pyproject.toml

@iainelder
Copy link

iainelder commented Oct 8, 2023

botocove depends on flakeheaven and pep8-naming.

I want botocove to use the latest version of its development dependencies.

flakeheaven 3.3.0 needs flake8 4.

pep8-naming 0.13.3 needs flake8 5.

So I can't install these latest versions together because they conflict over flake8.

To solve this specific conflict I'm going to ask the pep8-naming maintainer whether it can also support flake8 4. Otherwise, I may have to downgrade pep8-naming or find some other solution.

(I scored out the above because pep8-naming won't restore support for flake8 4. They formally dropped support for flake8 4 in PR 210 because it wasn't working anyway.)

If flake8 5 is the future then flakeheaven needs to adapt to it to remain relevant.

Use this pyproject.toml to reproduce the conflict with the Poetry dependency manager.

[tool.poetry]
name = "test"
version = "0"
description = ""
authors = [""]

[tool.poetry.dependencies]
python = "^3.8"

[tool.poetry.dev-dependencies]
pep8-naming = ">=0.13.3"
flakeheaven = ">=3.3.0"

[build-system]
requires = ["poetry-core"]
build-backend = "poetry.core.masonry.api"
$ poetry install
Updating dependencies
Resolving dependencies... (0.3s)

Because no versions of flakeheaven match >3.3.0
 and flakeheaven (3.3.0) depends on flake8 (>=4.0.1,<5.0.0), flakeheaven (>=3.3.0) requires flake8 (>=4.0.1,<5.0.0).
And because pep8-naming (0.13.3) depends on flake8 (>=5.0.0)
 and no versions of pep8-naming match >0.13.3, flakeheaven (>=3.3.0) is incompatible with pep8-naming (>=0.13.3).
So, because test depends on both pep8-naming (>=0.13.3) and flakeheaven (>=3.3.0), version solving failed.

@berzi
Copy link

berzi commented Oct 8, 2023

Not a real answer, but my solution was to simply switch to ruff completely.

@pwoolvett
Copy link

pwoolvett commented Oct 8, 2023

botocove depends on flakeheaven and pep8-naming.

I want botocove to use the latest version of its development dependencies.

flakeheaven 3.3.0 needs flake8 4.

pep8-naming 0.13.3 needs flake8 5.

So I can't install these latest versions together because they conflict over flake8.

To solve this specific conflict I'm going to ask the pep8-naming maintainer whether it can also support flake8 4. Otherwise, I may have to downgrade pep8-naming or find some other solution.

If flake8 5 is the future then flakeheaven needs to adapt to it to remain relevant.

Use this pyproject.toml to reproduce the conflict with the Poetry dependency manager.

[tool.poetry]
name = "test"
version = "0"
description = ""
authors = [""]

[tool.poetry.dependencies]
python = "^3.8"

[tool.poetry.dev-dependencies]
pep8-naming = ">=0.13.3"
flakeheaven = ">=3.3.0"

[build-system]
requires = ["poetry-core"]
build-backend = "poetry.core.masonry.api"
$ poetry install
Updating dependencies
Resolving dependencies... (0.3s)

Because no versions of flakeheaven match >3.3.0
 and flakeheaven (3.3.0) depends on flake8 (>=4.0.1,<5.0.0), flakeheaven (>=3.3.0) requires flake8 (>=4.0.1,<5.0.0).
And because pep8-naming (0.13.3) depends on flake8 (>=5.0.0)
 and no versions of pep8-naming match >0.13.3, flakeheaven (>=3.3.0) is incompatible with pep8-naming (>=0.13.3).
So, because test depends on both pep8-naming (>=0.13.3) and flakeheaven (>=3.3.0), version solving failed.

Sorry to hear that.
I'm focusing my efforts on ruff :), and don't have time anymore to make the required upgrades here, specially as the refactor for flake V5 is non trivial.

I suggest ruff, although depending on the complexity of your lint it might take you some time. Alternatively, as you mentioned pinnning down pep8-naming should work.

If you're willing to take a shot on the migration though, you're more than welcome!

@iainelder
Copy link

Thanks @berzi and @pwoolvett for the quick feedback to remind about ruff.

Now if even the flakeheaven maintainer recommends ruff I have a compelling reason to change 🙂

As a quick fix I'll use an older version of pep8-naming and then I'll look into migrating to ruff.

Thanks for all your lint support so far!

@berzi
Copy link

berzi commented Oct 8, 2023

In my experience the move to ruff is rather painless, it can even read the codes you have configured for flake8 & friends. The only issue is if you have custom linting rules, because ruff doesn't support those yet. Good luck with your project. :)

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

No branches or pull requests