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

Remove pylint configuration file #516

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

doolio
Copy link
Contributor

@doolio doolio commented Jan 20, 2024

Project now uses ruff for linting so this config file is obsolete.

Project now uses ruff for linting so this config file is obsolete.
@ccordoba12
Copy link
Member

ccordoba12 commented Jan 20, 2024

It seems pylintrc is used in a couple of tests. So you need to move it to the tests directory instead and update the respective tests.

@doolio
Copy link
Contributor Author

doolio commented Jan 20, 2024

But is that not because we still install pylint and pycodestyle? Should these have not been removed as pyflakes and black were?

# If we don't install pycodestyle, pylint will throw an unused-argument error in pylsp/plugins/pycodestyle_lint.py:72
# This error cannot be resolved by adding a pylint: disable=unused-argument comment ...
- run: |
pip install -e .[pylint,pycodestyle]
pip install ruff

cc: @tkrabel

@tkrabel
Copy link
Contributor

tkrabel commented Jan 21, 2024

But is that not because we still install pylint and pycodestyle? Should these have not been removed as pyflakes and black were?

# If we don't install pycodestyle, pylint will throw an unused-argument error in pylsp/plugins/pycodestyle_lint.py:72
# This error cannot be resolved by adding a pylint: disable=unused-argument comment ...
- run: |
pip install -e .[pylint,pycodestyle]
pip install ruff

cc: @tkrabel

I came to the conclusion that pylint is still used in some tests because of the pylint plugin. That’s why it needs to be installed, requiring also pycodestyle. I won’t resist removing that if you find a way to make all tests pass :)

@doolio
Copy link
Contributor Author

doolio commented Jan 22, 2024

I guess it makes sense we install pylint to test the pylint plugin but then I don't understand then why we no longer install pyflakes to test the pyflakes plugin (and perhaps the flake8 plugin)? Or the other tools (autopep8, mccabe, pydocstyle, rope and yapf for their respective plugins for that matter? I'm still a beginner so forgive my ignorance here.

In any case, I think we can still remove this config file and have a [tool.pylint] section in the pyproject.toml (which pylint supports) at the very least. This way we have only the one config file to maintain.

@ccordoba12
Copy link
Member

ccordoba12 commented Jan 22, 2024

@doolio, I think you're misunderstanding things a bit. If you look at one of the failing tests

imagen

you'll see its workspace is set at the root of the repo to get access to its pylintrc file. So, you need to move that file to another place inside our tests directory and update that test accordingly.

@doolio
Copy link
Contributor Author

doolio commented Jan 22, 2024

I think you're misunderstanding things a bit.

Very likely so I appreciate you taking the time. These tests are failing because I have removed the .pylintrc with this PR and presumably do not fail if it is present, right? So I think pylint searches up the directory tree until it finds its configuration file whether that be a .pylintrc or the pyproject.toml though the former takes precedence. Therefore, I think if I add the pylint configuration to the pyproject.toml the tests should pass If they were passing before, no?

My aim with this PR was to remove .pylintrc so we have less configuration to maintain.

@ccordoba12
Copy link
Member

ccordoba12 commented Jan 23, 2024

Therefore, I think if I add the pylint configuration to the pyproject.toml the tests should pass If they were passing before, no?

That should be the case, yes. But I don't think it's necessary because we're not using Pylint for the project anymore. That's why I propose to move the current pylintrc to our test folder and update the failing tests accordingly.

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