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

Disable Autopep8 and Yapf if this plugin is installed #34

Merged
merged 5 commits into from
Apr 12, 2022

Conversation

bagel897
Copy link
Contributor

This does 3 things:

  1. Disable YAPF
  2. Disable autopep8
  3. change the flake8 config to use a max line length of 88 instead of 80.

@haplo
Copy link
Collaborator

haplo commented Mar 30, 2022

This looks good to me, I will try it out later. What do you think @ccordoba12?

@ccordoba12
Copy link
Member

This doesn't need to be handled here. Instead, your editor or IDE @bageljrkhanofemus needs to send a workspace/didChangeConfiguration to disable the plugins that you don't want to use and change configuration for others.

Plugins like this one are only in charge of offering configuration options and setting some defaults for them, not to set specific values on behalf of users.

@bagel897
Copy link
Contributor Author

I was just wondering if there was a better solution to the conflicts caused by black - since it instantiates a line length of 88 while flake8 uses 80, causing alot of unneeded frustration. But if it is better to do so in your IDE/Editor configuration, could that be added that as an recommendation?

@ccordoba12
Copy link
Member

I was just wondering if there was a better solution to the conflicts caused by black

I think we could leave here the configuration entries that disable autopep8 and yapf, if @haplo agrees with it.

But if it is better to do so in your IDE/Editor configuration, could that be added that as an recommendation?

Sure, for flake8 that's the way to go.

@bagel897
Copy link
Contributor Author

Done, I'll move discussion about flake8 to here python-lsp/python-lsp-server#186

@haplo
Copy link
Collaborator

haplo commented Mar 30, 2022

I was just wondering if there was a better solution to the conflicts caused by black - since it instantiates a line length of 88 while flake8 uses 80, causing alot of unneeded frustration. But if it is better to do so in your IDE/Editor configuration, could that be added that as an recommendation?

I think the best way to handle this is through flake8's configuration files, so it would always work, not just python-lsp-server and not just the developers adjusting their IDE configuration.

From black documentation: https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html#flake8

@haplo
Copy link
Collaborator

haplo commented Mar 30, 2022

I was just wondering if there was a better solution to the conflicts caused by black

I think we could leave here the configuration entries that disable autopep8 and yapf, if @haplo agrees with it.

I think it's a good idea, I expect it would be useful to most users. However what would happen to an user that installed python-lsp-black globally but only tried to use it in some projects, while using yapf or autopep8 on others? Would manually overriding the configuration work? Whichever way we choose we should document the default behavior and how to override it.

@bagel897
Copy link
Contributor Author

bagel897 commented Mar 31, 2022

I was going to check how autopep8 and yapf handle it, when I noticed a couple of things

  1. While the docs say that yapf is preferred over autopep8, the implementation (if both are installed) is to first call autopep8, then yapf on the same file
  2. adding black to the mix (before the merge) would result in it being called first before the others, since we call it with a list of non-disabled plugins.

I wonder if there is a way to get the PluginManager from pluggy to disable other plugins at runtime

@ccordoba12 ccordoba12 changed the title fix plugin conflicts Disable Autopep8 and Yapf if this plugin is installed Apr 4, 2022
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks @bageljrkhanofemus!

@ccordoba12 ccordoba12 added this to the v1.2.1 milestone Apr 4, 2022
@haplo
Copy link
Collaborator

haplo commented Apr 8, 2022

@bageljrkhanofemus Could you update the README to mention that python-lsp-black will by default disable yapf and autopep8 plugins from python-lsp-server?

@bagel897
Copy link
Contributor Author

bagel897 commented Apr 8, 2022

done

@bagel897
Copy link
Contributor Author

You need to update black to 22.3.0 for github builds to work because black < 22.3.0 is broken with the latest version of click.. I don't see a dependency specification anywhere but you may want to specify it in a pyproject.toml file for PEP 517/518/621 compliant approach. I can help with that if you want me to.
See https://stackoverflow.com/questions/71673404/importerror-cannot-import-name-unicodefun-from-click for the error.

@ccordoba12
Copy link
Member

Here you can find the dependency on Black:

install_requires = python-lsp-server>=1.4.0; black>=22.1.0; toml

Please update it to 22.3.0. so we can release 1.2.1 with this fix asap.

@bagel897
Copy link
Contributor Author

done

@ccordoba12
Copy link
Member

Thanks @bageljrkhanofemus for your help to solve the Click issue!

@haplo, what do you say?

@haplo
Copy link
Collaborator

haplo commented Apr 11, 2022

This looks good to me, thank you @bageljrkhanofemus for your work on this!

I will merge tomorrow and release.

@haplo haplo merged commit e150769 into python-lsp:master Apr 12, 2022
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.

3 participants