pylint icon indicating copy to clipboard operation
pylint copied to clipboard

Search for pyproject.toml config file in parent dirs

Open Felixoid opened this issue 3 years ago â€Ē 38 comments

Type of Changes

Type
✓ :sparkles: New feature

Description

Search for pyproject.toml in parent directories.

Closes #3289

Felixoid avatar Jul 11 '22 16:07 Felixoid

Pull Request Test Coverage Report for Build 3014845916

  • 17 of 17 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 95.338%

Totals Coverage Status
Change from base Build 3008071684: 0.004%
Covered Lines: 17033
Relevant Lines: 17866

💛 - Coveralls

coveralls avatar Jul 11 '22 16:07 coveralls

How would I add exceptions for false-positive spell checker?

Felixoid avatar Jul 11 '22 16:07 Felixoid

Thanks for the interest in this issue @Felixoid. I didn't notice we left that issue open, but due to discussion in https://github.com/PyCQA/pylint/pull/3769 I am not sure if we can merge this PR. Even if we do want to I think we should consider whether this is a breaking change and if it can be included in 2.x.

DanielNoord avatar Jul 11 '22 17:07 DanielNoord

I think this should use the specification from the discussion in https://github.com/PyCQA/pylint/issues/3289

Pierre-Sassoulas avatar Jul 11 '22 17:07 Pierre-Sassoulas

How would I add exceptions for false-positive spell checker?

There's a dictionary called .pyenchant_pylint_custom_dict.txt at the root of the project.

Pierre-Sassoulas avatar Jul 11 '22 17:07 Pierre-Sassoulas

ðŸĪ– Effect of this PR on checked open source code: ðŸĪ–

Effect on astroid: The following messages are now emitted:

  1. unused-argument: Unused argument 'context' https://github.com/PyCQA/astroid/blob/main/astroid/objects.py#L345
  2. unused-argument: Unused argument 'kwargs' https://github.com/PyCQA/astroid/blob/main/astroid/objects.py#L345

Effect on django: The following messages are now emitted:

  1. too-many-instance-attributes: Too many instance attributes (22/11) https://github.com/django/django/blob/main/django/http/request.py#L47

The following messages are no longer emitted:

  1. too-many-instance-attributes: Too many instance attributes (20/11) https://github.com/django/django/blob/main/django/http/request.py#L47
  2. redefined-variable-type: Redefinition of field type from django.db.models.fields.related.OneToOneField to django.forms.widgets.Widget https://github.com/django/django/blob/main/django/db/models/base.py#L344

Effect on pandas: The following messages are now emitted:

  1. redefined-variable-type: Redefinition of result type from pandas.core.indexes.period.PeriodIndex to pandas.core.indexes.multi.MultiIndex https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/multi/test_reshape.py#L126
  2. redefined-variable-type: Redefinition of dti type from pandas.core.indexes.datetimes.DatetimeIndex to pandas.core.indexes.period.PeriodIndex https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/datetimes/test_misc.py#L248
  3. no-member: Instance of 'PeriodIndex' has no 'tz' member https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/datetimes/methods/test_shift.py#L101
  4. redefined-variable-type: Redefinition of indexer type from pandas.core.indexes.numeric.Int64Index to pandas.core.indexes.period.PeriodIndex https://github.com/pandas-dev/pandas/blob/main/pandas/core/algorithms.py#L1380
  5. too-many-instance-attributes: Too many instance attributes (31/11) https://github.com/pandas-dev/pandas/blob/main/pandas/core/frame.py#L468

The following messages are no longer emitted:

  1. no-member: Instance of 'DataFrame' has no 'one' member https://github.com/pandas-dev/pandas/blob/main/pandas/tests/frame/methods/test_matmul.py#L32
  2. no-member: Instance of 'DataFrame' has no 'one' member https://github.com/pandas-dev/pandas/blob/main/pandas/tests/frame/methods/test_matmul.py#L33
  3. redefined-variable-type: Redefinition of result type from pandas.core.indexes.datetimes.DatetimeIndex to pandas.core.indexes.multi.MultiIndex https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/multi/test_reshape.py#L126
  4. redefined-variable-type: Redefinition of indexer type from pandas.core.indexes.numeric.Int64Index to pandas.core.indexes.datetimes.DatetimeIndex https://github.com/pandas-dev/pandas/blob/main/pandas/core/algorithms.py#L1394
  5. too-many-instance-attributes: Too many instance attributes (33/11) https://github.com/pandas-dev/pandas/blob/main/pandas/core/frame.py#L468
  6. attribute-defined-outside-init: Attribute 'name' defined outside init https://github.com/pandas-dev/pandas/blob/main/pandas/core/frame.py#L9762

Effect on sentry: The following messages are now emitted:

  1. unsupported-binary-operation: unsupported operand type(s) for | https://github.com/getsentry/sentry/blob/master/src/sentry/api/endpoints/debug_files.py#L162
  2. too-many-instance-attributes: Too many instance attributes (20/11) https://github.com/getsentry/sentry/blob/master/src/sentry/mediators/param.py#L7
  3. unneeded-not: Consider changing "not settings.SENTRY_EVENTSTREAM == 'sentry.eventstream.kafka.KafkaEventStream'" to "settings.SENTRY_EVENTSTREAM != 'sentry.eventstream.kafka.KafkaEventStream'" https://github.com/getsentry/sentry/blob/master/src/sentry/runner/commands/devserver.py#L251
  4. unneeded-not: Consider changing "not settings.SENTRY_EVENTSTREAM == 'sentry.eventstream.kafka.KafkaEventStream'" to "settings.SENTRY_EVENTSTREAM != 'sentry.eventstream.kafka.KafkaEventStream'" https://github.com/getsentry/sentry/blob/master/src/sentry/runner/commands/devserver.py#L260

The following messages are no longer emitted:

  1. unused-import: Unused Organization imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/testutils/fixtures.py#L7
  2. unused-import: Unused Organization imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/integrations/slack/utils/channel.py#L8
  3. unused-import: Unused Project imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/api/helpers/group_index/delete.py#L11
  4. unused-import: Unused Organization imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/api/helpers/group_index/index.py#L15
  5. unused-import: Unused Project imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/api/helpers/group_index/index.py#L15
  6. unsupported-binary-operation: unsupported operand type(s) for | https://github.com/getsentry/sentry/blob/master/src/sentry/api/endpoints/user_index.py#L27
  7. import-error: Unable to import 'sentry.utils.distutils.commands.base' https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L13
  8. missing-function-docstring: Missing function or method docstring https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L42
  9. no-self-use: Method could be a function https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L42
  10. too-few-public-methods: Too few public methods (1/2) https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L39
  11. import-error: Unable to import 'sentry.utils.distutils.commands.base' https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_integration_docs.py#L4
  12. missing-function-docstring: Missing function or method docstring https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_integration_docs.py#L10
  13. too-few-public-methods: Too few public methods (1/2) https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_integration_docs.py#L7

This comment was generated for commit d5f99f6560afff9d88857af3dcfa22f6807db691

github-actions[bot] avatar Jul 11 '22 17:07 github-actions[bot]

Hi @DanielNoord, there's nothing relatively new in the code that wasn't there already. The snippet curdir = Path(os.getcwd()).resolve() is a bit below, so it should work fine.

Dear @Pierre-Sassoulas, you mean that .git/.hg as another marker for the pyproject.toml directory? Or the max depth? Or rather that it should search for any possible configuration up to a repository root directory?

Felixoid avatar Jul 11 '22 17:07 Felixoid

We handle multiple configuration file (pylintrc/.pylintrc / pyproject.toml, setup.cfg too) I think we should upgrade the current function to search for configuration files to make it search up the tree until it hits a configuration file or a .git / .hg directory. Unless I'm mistaken it's this function that need to be modified:

https://github.com/PyCQA/pylint/blob/2e2f8f8c12a2e54c18c0e4dd49e4850f5c913b1f/pylint/config/find_default_config_files.py#L42

Pierre-Sassoulas avatar Jul 11 '22 18:07 Pierre-Sassoulas

Thank you for your review, I've added processing of all supported configs, and updated the docs

Felixoid avatar Jul 11 '22 21:07 Felixoid

ðŸĪ– Effect of this PR on checked open source code: ðŸĪ–

Effect on astroid: The following messages are now emitted:

  1. unused-argument: Unused argument 'context' https://github.com/PyCQA/astroid/blob/main/astroid/objects.py#L345
  2. unused-argument: Unused argument 'kwargs' https://github.com/PyCQA/astroid/blob/main/astroid/objects.py#L345

Effect on django: The following messages are now emitted:

  1. too-many-instance-attributes: Too many instance attributes (22/11) https://github.com/django/django/blob/main/django/http/request.py#L47

The following messages are no longer emitted:

  1. too-many-instance-attributes: Too many instance attributes (20/11) https://github.com/django/django/blob/main/django/http/request.py#L47
  2. redefined-variable-type: Redefinition of field type from django.db.models.fields.related.OneToOneField to django.forms.widgets.Widget https://github.com/django/django/blob/main/django/db/models/base.py#L344

Effect on pandas: The following messages are now emitted:

  1. redefined-variable-type: Redefinition of result type from pandas.core.indexes.period.PeriodIndex to pandas.core.indexes.multi.MultiIndex https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/multi/test_reshape.py#L126
  2. redefined-variable-type: Redefinition of dti type from pandas.core.indexes.datetimes.DatetimeIndex to pandas.core.indexes.period.PeriodIndex https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/datetimes/test_misc.py#L248
  3. no-member: Instance of 'PeriodIndex' has no 'tz' member https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/datetimes/methods/test_shift.py#L101
  4. redefined-variable-type: Redefinition of indexer type from pandas.core.indexes.numeric.Int64Index to pandas.core.indexes.period.PeriodIndex https://github.com/pandas-dev/pandas/blob/main/pandas/core/algorithms.py#L1380
  5. too-many-instance-attributes: Too many instance attributes (31/11) https://github.com/pandas-dev/pandas/blob/main/pandas/core/frame.py#L468

The following messages are no longer emitted:

  1. no-member: Instance of 'DataFrame' has no 'one' member https://github.com/pandas-dev/pandas/blob/main/pandas/tests/frame/methods/test_matmul.py#L32
  2. no-member: Instance of 'DataFrame' has no 'one' member https://github.com/pandas-dev/pandas/blob/main/pandas/tests/frame/methods/test_matmul.py#L33
  3. redefined-variable-type: Redefinition of result type from pandas.core.indexes.datetimes.DatetimeIndex to pandas.core.indexes.multi.MultiIndex https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/multi/test_reshape.py#L126
  4. redefined-variable-type: Redefinition of indexer type from pandas.core.indexes.numeric.Int64Index to pandas.core.indexes.datetimes.DatetimeIndex https://github.com/pandas-dev/pandas/blob/main/pandas/core/algorithms.py#L1394
  5. too-many-instance-attributes: Too many instance attributes (33/11) https://github.com/pandas-dev/pandas/blob/main/pandas/core/frame.py#L468
  6. attribute-defined-outside-init: Attribute 'name' defined outside init https://github.com/pandas-dev/pandas/blob/main/pandas/core/frame.py#L9762

Effect on sentry: The following messages are now emitted:

  1. unsupported-binary-operation: unsupported operand type(s) for | https://github.com/getsentry/sentry/blob/master/src/sentry/api/endpoints/debug_files.py#L162
  2. too-many-instance-attributes: Too many instance attributes (20/11) https://github.com/getsentry/sentry/blob/master/src/sentry/mediators/param.py#L7
  3. unneeded-not: Consider changing "not settings.SENTRY_EVENTSTREAM == 'sentry.eventstream.kafka.KafkaEventStream'" to "settings.SENTRY_EVENTSTREAM != 'sentry.eventstream.kafka.KafkaEventStream'" https://github.com/getsentry/sentry/blob/master/src/sentry/runner/commands/devserver.py#L251
  4. unneeded-not: Consider changing "not settings.SENTRY_EVENTSTREAM == 'sentry.eventstream.kafka.KafkaEventStream'" to "settings.SENTRY_EVENTSTREAM != 'sentry.eventstream.kafka.KafkaEventStream'" https://github.com/getsentry/sentry/blob/master/src/sentry/runner/commands/devserver.py#L260

The following messages are no longer emitted:

  1. unused-import: Unused Organization imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/testutils/fixtures.py#L7
  2. unused-import: Unused Organization imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/integrations/slack/utils/channel.py#L8
  3. unused-import: Unused Project imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/api/helpers/group_index/delete.py#L11
  4. unused-import: Unused Organization imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/api/helpers/group_index/index.py#L15
  5. unused-import: Unused Project imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/api/helpers/group_index/index.py#L15
  6. unsupported-binary-operation: unsupported operand type(s) for | https://github.com/getsentry/sentry/blob/master/src/sentry/api/endpoints/user_index.py#L27
  7. import-error: Unable to import 'sentry.utils.distutils.commands.base' https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L13
  8. missing-function-docstring: Missing function or method docstring https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L42
  9. no-self-use: Method could be a function https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L42
  10. too-few-public-methods: Too few public methods (1/2) https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L39
  11. import-error: Unable to import 'sentry.utils.distutils.commands.base' https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_integration_docs.py#L4
  12. missing-function-docstring: Missing function or method docstring https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_integration_docs.py#L10
  13. too-few-public-methods: Too few public methods (1/2) https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_integration_docs.py#L7

This comment was generated for commit f7dc2042c853bd9d7bcdaf4a2164fb308e26078e

github-actions[bot] avatar Jul 11 '22 21:07 github-actions[bot]

It definitely went out of control. I'll take care of it.

Felixoid avatar Jul 11 '22 21:07 Felixoid

The failing tests show that this is not a trivial change. Again, I'm against changing this in a 2.x update. Consider:

A directory without a git repo and no pyproject.toml. For example, a collection of (internal) scripts. An user can now run pylint on this directory and get the default values and pass the checks. With this change we would now start going up until the user's root, thereby potentially changing the config file that is being used for their checks.

I'm usually fine with having users change some configuration stuff between minor patches, but forcing them to restructure their directories or move files around doesn't seem like a good thing to do in a minor version update.

DanielNoord avatar Jul 11 '22 21:07 DanielNoord

Ok, I misinterpret your first message. I thought that you've meant python2.

You are right on one hand. On another, now nobody can set a project global pylintrc, of there is a mixed repo. That's why I started working on the issue, basically

Felixoid avatar Jul 11 '22 21:07 Felixoid

Ok, I misinterpret your first message. I thought that you've meant python2.

You are right on one hand. On another, now nobody can set a project global pylintrc, of there is a mixed repo. That's why I started working on the issue, basically

See L62-80 of the original code. We currently support an environment variable and a global config in the users home directory. This probably doesn't cover all cases (and it might still make sense to merge this PR at some point), but I'm not sure we can change this now.

I haven't tested this myself but can't you also use pylint --rcfile=../../../pylintrc?

DanielNoord avatar Jul 11 '22 21:07 DanielNoord

ðŸĪ– Effect of this PR on checked open source code: ðŸĪ–

Effect on astroid: The following messages are now emitted:

  1. unused-argument: Unused argument 'context' https://github.com/PyCQA/astroid/blob/main/astroid/objects.py#L345
  2. unused-argument: Unused argument 'kwargs' https://github.com/PyCQA/astroid/blob/main/astroid/objects.py#L345

Effect on django: The following messages are now emitted:

  1. too-many-instance-attributes: Too many instance attributes (22/11) https://github.com/django/django/blob/main/django/http/request.py#L47

The following messages are no longer emitted:

  1. too-many-instance-attributes: Too many instance attributes (20/11) https://github.com/django/django/blob/main/django/http/request.py#L47
  2. redefined-variable-type: Redefinition of field type from django.db.models.fields.related.OneToOneField to django.forms.widgets.Widget https://github.com/django/django/blob/main/django/db/models/base.py#L344

Effect on pandas: The following messages are now emitted:

  1. redefined-variable-type: Redefinition of result type from pandas.core.indexes.period.PeriodIndex to pandas.core.indexes.multi.MultiIndex https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/multi/test_reshape.py#L126
  2. redefined-variable-type: Redefinition of dti type from pandas.core.indexes.datetimes.DatetimeIndex to pandas.core.indexes.period.PeriodIndex https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/datetimes/test_misc.py#L248
  3. no-member: Instance of 'PeriodIndex' has no 'tz' member https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/datetimes/methods/test_shift.py#L101
  4. redefined-variable-type: Redefinition of indexer type from pandas.core.indexes.numeric.Int64Index to pandas.core.indexes.period.PeriodIndex https://github.com/pandas-dev/pandas/blob/main/pandas/core/algorithms.py#L1380
  5. too-many-instance-attributes: Too many instance attributes (31/11) https://github.com/pandas-dev/pandas/blob/main/pandas/core/frame.py#L468

The following messages are no longer emitted:

  1. no-member: Instance of 'DataFrame' has no 'one' member https://github.com/pandas-dev/pandas/blob/main/pandas/tests/frame/methods/test_matmul.py#L32
  2. no-member: Instance of 'DataFrame' has no 'one' member https://github.com/pandas-dev/pandas/blob/main/pandas/tests/frame/methods/test_matmul.py#L33
  3. redefined-variable-type: Redefinition of result type from pandas.core.indexes.datetimes.DatetimeIndex to pandas.core.indexes.multi.MultiIndex https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/multi/test_reshape.py#L126
  4. redefined-variable-type: Redefinition of indexer type from pandas.core.indexes.numeric.Int64Index to pandas.core.indexes.datetimes.DatetimeIndex https://github.com/pandas-dev/pandas/blob/main/pandas/core/algorithms.py#L1394
  5. too-many-instance-attributes: Too many instance attributes (33/11) https://github.com/pandas-dev/pandas/blob/main/pandas/core/frame.py#L468
  6. attribute-defined-outside-init: Attribute 'name' defined outside init https://github.com/pandas-dev/pandas/blob/main/pandas/core/frame.py#L9762

Effect on sentry: The following messages are now emitted:

  1. unsupported-binary-operation: unsupported operand type(s) for | https://github.com/getsentry/sentry/blob/master/src/sentry/api/endpoints/debug_files.py#L162
  2. too-many-instance-attributes: Too many instance attributes (20/11) https://github.com/getsentry/sentry/blob/master/src/sentry/mediators/param.py#L7
  3. unneeded-not: Consider changing "not settings.SENTRY_EVENTSTREAM == 'sentry.eventstream.kafka.KafkaEventStream'" to "settings.SENTRY_EVENTSTREAM != 'sentry.eventstream.kafka.KafkaEventStream'" https://github.com/getsentry/sentry/blob/master/src/sentry/runner/commands/devserver.py#L251
  4. unneeded-not: Consider changing "not settings.SENTRY_EVENTSTREAM == 'sentry.eventstream.kafka.KafkaEventStream'" to "settings.SENTRY_EVENTSTREAM != 'sentry.eventstream.kafka.KafkaEventStream'" https://github.com/getsentry/sentry/blob/master/src/sentry/runner/commands/devserver.py#L260

The following messages are no longer emitted:

  1. unused-import: Unused Organization imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/testutils/fixtures.py#L7
  2. unused-import: Unused Organization imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/integrations/slack/utils/channel.py#L8
  3. unused-import: Unused Project imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/api/helpers/group_index/delete.py#L11
  4. unused-import: Unused Organization imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/api/helpers/group_index/index.py#L15
  5. unused-import: Unused Project imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/api/helpers/group_index/index.py#L15
  6. unsupported-binary-operation: unsupported operand type(s) for | https://github.com/getsentry/sentry/blob/master/src/sentry/api/endpoints/user_index.py#L27
  7. import-error: Unable to import 'sentry.utils.distutils.commands.base' https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L13
  8. missing-function-docstring: Missing function or method docstring https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L42
  9. no-self-use: Method could be a function https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L42
  10. too-few-public-methods: Too few public methods (1/2) https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L39
  11. import-error: Unable to import 'sentry.utils.distutils.commands.base' https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_integration_docs.py#L4
  12. missing-function-docstring: Missing function or method docstring https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_integration_docs.py#L10
  13. too-few-public-methods: Too few public methods (1/2) https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_integration_docs.py#L7

This comment was generated for commit 6e9caa37ddc804ca34b458dc5b7421b4213fe9cd

github-actions[bot] avatar Jul 11 '22 22:07 github-actions[bot]

It's the morning, so I'll address all your questions.

With this change we would now start going up until the user's root, thereby potentially changing the config file that is being used for their checks.

It's precisely the issue I am trying to solve. And to show that it works, we already have the best proof. Take a look on the report. Each tested project has less number of reports than it has. It works as expected.

See L62-80 of the original code

Unfortunately, it's not even a workaround to have a repo-wide config. One can't set it for integrations, like me using ale for vim.

What is in my mind for now:

pylint has a severe issue with having a really basic feature of setting a repo-wide config. I 100% see your point with breaking change of looking for all types of config through the directories up to the root. What can be done, for now I'd roll back to the approximate state from the commit and add .git/.hg there only for pyproject.toml configuration. It's not such a significant change, and one is looking for a way to set a repo-wide config will have it.

The major change to search all configs in the directories tree may be then addressed in a major release.

Does it sound reasonable?

Felixoid avatar Jul 12 '22 07:07 Felixoid

It's precisely the issue I am trying to solve. And to show that it works, we already have the best proof. Take a look on the report. Each tested project has less number of reports than it has. It works as expected.

This is an unrelated report and caused by a bug in our primer. Actually I would expect this PR to have no effect in the primer as all primer projects are linted with a specified pylintrc. For now we should ignore all primer comments in this PR until #7160 has been merged.

What can be done, for now I'd roll back to the approximate state from the commit and add .git/.hg there only for pyproject.toml configuration. It's not such a significant change, and one is looking for a way to set a repo-wide config will have it.

The major change to search all configs in the directories tree may be then addressed in a major release.

Does it sound reasonable?

This is still a breaking change for those who want to lint subdirectories with default values. I can't stress enough the range of different pylint invocations. Take for example https://github.com/PyCQA/pylint/issues/6799, which shows that some users actually run pylint twice over the same repository with the same configuration file but with different command line arguments. Initially that seemed totally bizarre to me, but I guess people find a use case for it. Same goes for linting subdirectories with default values, we just don't know whether users rely on this.

I can see the need for repository wide configuration files, but I don't think this can be default value in a minor release. Perhaps we can work with a flag? --look-for-parent-configuration or something?

DanielNoord avatar Jul 12 '22 08:07 DanielNoord

Sorry, I can't call it breaking changes. For me, it is still an essential function that wasn't implemented for historical reasons.

The point is that each tool I can recall, e.g., black and isort (see the issue), flake8 already supports it at least somehow.

I hear your point. I can only agree to disagree. I see people need the same thing as I do, so this PR solves the issue that has been hanging around for three years.

What I can do more here is write tests to put to stone the proposed behavior with pyproject.toml or .git or .hg in a directory above. I wouldn't invest my time to invest then even more time into wrapping around workarounds.

update: tests are added in e24f7d1

Felixoid avatar Jul 12 '22 08:07 Felixoid

ðŸĪ– According to the primer, this change has no effect on the checked open source code. ðŸĪ–🎉

This comment was generated for commit 1599b2cc7f7a36eb0707826aa4eaec108fba2c50

github-actions[bot] avatar Jul 12 '22 09:07 github-actions[bot]

ðŸĪ– Effect of this PR on checked open source code: ðŸĪ–

Effect on astroid: The following messages are no longer emitted:

  1. unused-argument: Unused argument 'context' https://github.com/PyCQA/astroid/blob/main/astroid/objects.py#L345
  2. unused-argument: Unused argument 'kwargs' https://github.com/PyCQA/astroid/blob/main/astroid/objects.py#L345

Effect on django: The following messages are now emitted:

  1. too-many-instance-attributes: Too many instance attributes (20/11) https://github.com/django/django/blob/main/django/http/request.py#L47
  2. redefined-variable-type: Redefinition of field type from django.db.models.fields.related.OneToOneField to django.forms.widgets.Widget https://github.com/django/django/blob/main/django/db/models/base.py#L344

The following messages are no longer emitted:

  1. too-many-instance-attributes: Too many instance attributes (22/11) https://github.com/django/django/blob/main/django/http/request.py#L47

Effect on pandas: The following messages are now emitted:

  1. no-member: Instance of 'DataFrame' has no 'one' member https://github.com/pandas-dev/pandas/blob/main/pandas/tests/frame/methods/test_matmul.py#L32
  2. no-member: Instance of 'DataFrame' has no 'one' member https://github.com/pandas-dev/pandas/blob/main/pandas/tests/frame/methods/test_matmul.py#L33
  3. redefined-variable-type: Redefinition of result type from pandas.core.indexes.datetimes.DatetimeIndex to pandas.core.indexes.multi.MultiIndex https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/multi/test_reshape.py#L126
  4. redefined-variable-type: Redefinition of indexer type from pandas.core.indexes.numeric.Int64Index to pandas.core.indexes.datetimes.DatetimeIndex https://github.com/pandas-dev/pandas/blob/main/pandas/core/algorithms.py#L1394
  5. too-many-instance-attributes: Too many instance attributes (33/11) https://github.com/pandas-dev/pandas/blob/main/pandas/core/frame.py#L468
  6. attribute-defined-outside-init: Attribute 'name' defined outside init https://github.com/pandas-dev/pandas/blob/main/pandas/core/frame.py#L9762

The following messages are no longer emitted:

  1. redefined-variable-type: Redefinition of result type from pandas.core.indexes.period.PeriodIndex to pandas.core.indexes.multi.MultiIndex https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/multi/test_reshape.py#L126
  2. redefined-variable-type: Redefinition of dti type from pandas.core.indexes.datetimes.DatetimeIndex to pandas.core.indexes.period.PeriodIndex https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/datetimes/test_misc.py#L248
  3. no-member: Instance of 'PeriodIndex' has no 'tz' member https://github.com/pandas-dev/pandas/blob/main/pandas/tests/indexes/datetimes/methods/test_shift.py#L101
  4. redefined-variable-type: Redefinition of indexer type from pandas.core.indexes.numeric.Int64Index to pandas.core.indexes.period.PeriodIndex https://github.com/pandas-dev/pandas/blob/main/pandas/core/algorithms.py#L1380
  5. too-many-instance-attributes: Too many instance attributes (31/11) https://github.com/pandas-dev/pandas/blob/main/pandas/core/frame.py#L468

Effect on sentry: The following messages are now emitted:

  1. unused-import: Unused Organization imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/testutils/fixtures.py#L7
  2. unused-import: Unused Organization imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/integrations/slack/utils/channel.py#L8
  3. unused-import: Unused Project imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/api/helpers/group_index/delete.py#L11
  4. unused-import: Unused Organization imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/api/helpers/group_index/index.py#L15
  5. unused-import: Unused Project imported from sentry.models https://github.com/getsentry/sentry/blob/master/src/sentry/api/helpers/group_index/index.py#L15
  6. unsupported-binary-operation: unsupported operand type(s) for | https://github.com/getsentry/sentry/blob/master/src/sentry/api/endpoints/user_index.py#L27
  7. import-error: Unable to import 'sentry.utils.distutils.commands.base' https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L13
  8. missing-function-docstring: Missing function or method docstring https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L42
  9. no-self-use: Method could be a function https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L42
  10. too-few-public-methods: Too few public methods (1/2) https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_js_sdk_registry.py#L39
  11. import-error: Unable to import 'sentry.utils.distutils.commands.base' https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_integration_docs.py#L4
  12. missing-function-docstring: Missing function or method docstring https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_integration_docs.py#L10
  13. too-few-public-methods: Too few public methods (1/2) https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/build_integration_docs.py#L7

The following messages are no longer emitted:

  1. unsupported-binary-operation: unsupported operand type(s) for | https://github.com/getsentry/sentry/blob/master/src/sentry/api/endpoints/debug_files.py#L162
  2. too-many-instance-attributes: Too many instance attributes (20/11) https://github.com/getsentry/sentry/blob/master/src/sentry/mediators/param.py#L7
  3. unneeded-not: Consider changing "not settings.SENTRY_EVENTSTREAM == 'sentry.eventstream.kafka.KafkaEventStream'" to "settings.SENTRY_EVENTSTREAM != 'sentry.eventstream.kafka.KafkaEventStream'" https://github.com/getsentry/sentry/blob/master/src/sentry/runner/commands/devserver.py#L251
  4. unneeded-not: Consider changing "not settings.SENTRY_EVENTSTREAM == 'sentry.eventstream.kafka.KafkaEventStream'" to "settings.SENTRY_EVENTSTREAM != 'sentry.eventstream.kafka.KafkaEventStream'" https://github.com/getsentry/sentry/blob/master/src/sentry/runner/commands/devserver.py#L260

This comment was generated for commit e24f7d1fbd7040a59aa4b89b8140c4e5f293d83b

github-actions[bot] avatar Jul 12 '22 12:07 github-actions[bot]

This discussion has gotten stuck I think. I'm tagging some other active maintainers to gather more opinions about this.

@Pierre-Sassoulas @jacobtylerwalls

DanielNoord avatar Aug 09 '22 21:08 DanielNoord

Hey @Felixoid, thanks for these changes. I think everyone is positive on your changes, we're just trying to figure out what release to put it in.

The good news is that even if it goes in 3.0, we'll probably put out a pre-release, and you can start using it. As I said here, merging great patches to a 3.0 branch just increases the urgency of releasing it. :-)

I see the argument on both sides. Honestly, this is what I thought the configurations were:

I can see the need for repository wide configuration files

But @DanielNoord has a point that people's CI workflows will break given that often they don't even know what their cwd is. (Even in developing pylint itself recently we've had several issues that hinged on cwd-sensitivity.) And people can get cross when we make changes that affect how they invoke pylint.

I don't feel too strongly either way, but I just want to reiterate that this part of the discussion should not slow down reviewing the changes and getting them merged either to main or to 3.0.

jacobtylerwalls avatar Aug 12 '22 00:08 jacobtylerwalls

merging great patches to a 3.0 branch just increases the urgency of releasing it.

👍 Strong agree. We have a LOT to release in 3.0, we need to add things that sparkle joy or we'll never ship it 😄

But this is not the reason I'm slightly on the side of doing the feature as a breaking change here. Environment bugs are really nasty to tests, troubleshoot, release, and get right in general. We can't anticipate all the possible environments used (we had solaris and another really niche OS recently, there's probably a Mars Rover prototype with custom hardware somewhere too). This is probably simpler in this case, which is why I'm only slightly in favor, but there's a very high probability that we're going to break something somewhere and this is not good practice to take that risk in a minor (especially as pylint add new messages with new checkers and fixed false negatives, so the migration to a new minor is not painless already).

Pierre-Sassoulas avatar Aug 13 '22 09:08 Pierre-Sassoulas

Thank you for your feedback, I got your point. I agree to redo it as a major change.

Unfortunately, I got serious sick today, and wouldn't be able to do it this week. Maybe, the weekend sounds realistic.

Felixoid avatar Aug 15 '22 15:08 Felixoid

As far as I see, the PR currently has a base main, so I guess it will end up in the 3.0, right?

Felixoid avatar Aug 19 '22 22:08 Felixoid

As far as I see, the PR currently has a base main, so I guess it will end up in the 3.0, right?

We're putting everything from main in 3.0 right now, yes.

Pierre-Sassoulas avatar Aug 21 '22 05:08 Pierre-Sassoulas

ðŸĪ– According to the primer, this change has no effect on the checked open source code. ðŸĪ–🎉

This comment was generated for commit 0d1da49689c0c2c22e56e50838b6c037921b0b92

github-actions[bot] avatar Aug 21 '22 06:08 github-actions[bot]

Hello again.

Is there anything preventing it from merging now?

Felixoid avatar Aug 30 '22 12:08 Felixoid

I added the need review label, but I don't have the availability right now :)

Pierre-Sassoulas avatar Aug 31 '22 08:08 Pierre-Sassoulas

I think the main issue is that we don't really have branch to put this on. We recently removed the 3.x branch as it took effort to keep it up to date while we are no where near releasing 3.x currently.

DanielNoord avatar Aug 31 '22 08:08 DanielNoord