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

Register Refurb as a Flake8 plugin #72

Closed
wants to merge 1 commit into from

Conversation

jdevera
Copy link
Contributor

@jdevera jdevera commented Oct 15, 2022

📖 What is this PR about?

Fixes #21

In #21 it was suggested Refurb could be packaged as a Flake8 extension.

This PR tries that.

FRB is chosen as error-code, similar to the existing FURB code, but Flake8 has a max prefix length of 3.

🤔 Some learnings

  • Flake8 supports extensions that will be called once per file or once per line. I chose one per file here, obviously.
  • Running Refurb like this is very slow. I have not checked why but calling refurb on a directory is much faster than calling it on each file of the directory.

🔕 Disabled by default

The plugin is disabled by default and thus requires running flake8 with --enable-extensions FRB

When the flake8 plugin for a particular tool is packaged separately, it's okay to enable it by default. We can interpret its installation as an explicit desire to run it as part of flake8.

However, including the plugin as part of the tool itself, like it is done in the PR, does not allow us to reach the same conclusion. Somebody might install Refurb to run it standalone and just by doing this, we should make subsequent flake8 runs slower.

Even if running Refurb were extremely fast, I think activation should still be explicit.

Alternatively, we could have a separate package to provide the plugin.

The plugin is disabled by default and thus requires running flake8 with
--enable-extensions FRB

FRB is chosen as error code as it is similar to existing FURB code but
Flake8 has a max prefix length of 3.
@jdevera jdevera marked this pull request as draft October 15, 2022 22:37
@dsully
Copy link

dsully commented Oct 20, 2022

I was just coming here to see about adding stdin and JSON output for refurb in order to have it work with a language server like https://github.com/jose-elias-alvarez/null-ls.nvim - this change would work however, since flake8 is already something that can be run via null-ls and is a common pre-merge validation.

Thanks for this change, hoping it will be merged!

@dosisod
Copy link
Owner

dosisod commented Oct 20, 2022

@jdevera I think the reason Refurb is running so slow is because it is using Mypy's fine grained cache, which, from what I can tell, does not get stored/retrieved from disk properly. I can't find the documentation, but I remember reading that Mypy (under certain conditions) will opt to regenerate the fine grained cache as opposed to using what is on disk. Might be wrong on that.

This was one of the reasons I didn't create Refurb as a Flake8 plugin, because you don't get as much control over how your code runs.

Some ideas for speeding things up:

  • Communicate to the mypy daemon in order to speed up queries. We might be able to do this for normal Refurb as well, but just for Flake8 would be a start. I don't know if this will actually speed things up or not, but it might.
  • Poke around in the mypy configuration options.

It is unfortunate that Flake8 doesn't let you run your plugin once per project instead of once per file/line. I guess they want to force you to write concurrent/optimized plugins, but that sometimes isn't feasible (such as with Mypy).

@jdevera jdevera closed this Aug 21, 2024
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.

Package as a Flake 8 Plugin
3 participants