Skip to content

Support globbing patterns for source-roots - #8281

Merged
Pierre-Sassoulas merged 2 commits into
pylint-dev:mainfrom
alexey-pelykh:feature/source-roots-globbing
Feb 21, 2023
Merged

Support globbing patterns for source-roots#8281
Pierre-Sassoulas merged 2 commits into
pylint-dev:mainfrom
alexey-pelykh:feature/source-roots-globbing

Conversation

@alexey-pelykh

@alexey-pelykh alexey-pelykh commented Feb 13, 2023

Copy link
Copy Markdown
Contributor

Add globbing pattern support for source-roots.

Type of Changes

Type
✨ New feature

Description

Closes #8290

@github-actions

This comment has been minimized.

@ollie-iterators

Copy link
Copy Markdown
Contributor

#8286 fixed the issue with self-import

@alexey-pelykh
alexey-pelykh force-pushed the feature/source-roots-globbing branch 2 times, most recently from 6219d68 to c33aef9 Compare February 14, 2023 08:31
@alexey-pelykh
alexey-pelykh marked this pull request as ready for review February 14, 2023 08:32
@codecov

codecov Bot commented Feb 14, 2023

Copy link
Copy Markdown

Codecov Report

Merging #8281 (f3d3e55) into main (e64f043) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8281      +/-   ##
==========================================
- Coverage   95.46%   95.46%   -0.01%     
==========================================
  Files         177      177              
  Lines       18704    18703       -1     
==========================================
- Hits        17856    17855       -1     
  Misses        848      848              
Impacted Files Coverage Δ
pylint/lint/base_options.py 100.00% <ø> (ø)
pylint/pyreverse/main.py 93.87% <ø> (ø)
pylint/config/argument.py 100.00% <100.00%> (ø)
pylint/testutils/reporter_for_tests.py 93.02% <0.00%> (-0.32%) ⬇️

@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.0.0 milestone Feb 14, 2023
@DanielNoord

Copy link
Copy Markdown
Collaborator

I haven't read much of the discussion but was triggered about the comment about breaking changes in the accompanying issue. Note that the current implementation is definitely a breaking change as it changes the behaviour of an existing option. That's what we normally consider to be a breaking change.

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

@DanielNoord it's not a breaking change since it's backwards compatible and on top the option in question hasn't been released yet as well

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

@Pierre-Sassoulas not a WIP anymore, yet there's a regression in test coverage since _paths_csv_transformer is no longer used. I'm not sure it's worth removing it really yet I'm open for options

Also, it would be much appreciated if this will go into the same release as the source-roots feature 🙏

@DudeNr33

Copy link
Copy Markdown
Collaborator

I really like the idea behind this.
However I share Pierre's opinion that this can get quite confusing for users now as this would only be available for the new --source-roots option, but not for others.

Users that learn glob patterns work for --source-roots might try to use the same for other options like --ignore-paths or simply when calling pylint (you could think of pylint --recursive=y src/prefix**/*.py or something similar) and spend quite some time trying to figure out why glob patterns work for one option but not for another.

A more holistic approach that takes into account the other options/arguments that accept paths would be better in my opinion.

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

@DudeNr33 I agree and adding globbing support for the input sources is easy and non-breaking, yet ignore-paths e.g. are regexp-based right now so that would be breaking unless we add a new set of parameters. What would be your vision on overcoming that?

@DudeNr33

Copy link
Copy Markdown
Collaborator

@DudeNr33 I agree and adding globbing support for the input sources is easy and non-breaking, yet ignore-paths e.g. are regexp-based right now so that would be breaking unless we add a new set of parameters. What would be your vision on overcoming that?

My take on this would be:

  • support globbing where the current behaviour is not altered (i.e. anything that is not regex based probably)
  • leave the regex based options as they are for v2.x
  • change the default behavior from regex to globbing in v3.0, optionally adding support for people who really must use regex patterns by adding --ignore-paths-regex etc.
  • nice to have in v2.x: warn the user if an expression that is currently regex based cannot be evaluated as glob pattern, and make them aware that they must use regex pattern here

@DanielNoord your opinion?

@DanielNoord

Copy link
Copy Markdown
Collaborator

I think it is better to do globbing now for those settings where it is not a breaking change (this one). It just makes it easier for users to transition to 3.0 as they would need to update one less config option.

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

@DudeNr33 @DanielNoord @Pierre-Sassoulas so, I've created #8310 and #8311 that can be addressed separately and would allow to unblock this PR.

@alexey-pelykh
alexey-pelykh force-pushed the feature/source-roots-globbing branch 2 times, most recently from 0c7b6e9 to b2ae3d5 Compare February 17, 2023 18:10
@alexey-pelykh
alexey-pelykh force-pushed the feature/source-roots-globbing branch from b2ae3d5 to f58d448 Compare February 17, 2023 18:45
@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

I guess I need some assistance with taming the CI as it fails randomly 🤔

DudeNr33
DudeNr33 previously approved these changes Feb 20, 2023

@DudeNr33 DudeNr33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I re-ran the CI jobs and they passed now.
I only got one minor remark on the changelog entry, but that is not a must.

Comment thread doc/whatsnew/fragments/8290.feature Outdated
Co-authored-by: Andreas Finkler <3929834+DudeNr33@users.noreply.github.com>
@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

Thanks, I've merged the suggestion

@github-actions

Copy link
Copy Markdown
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit f3d3e55

@DudeNr33 DudeNr33 added Enhancement ✨ Improvement to a component and removed Work in progress labels Feb 21, 2023

@DudeNr33 DudeNr33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

@Pierre-Sassoulas with the follow-up tasks for 3.0 created I think we can put this MR in the 2.17 release. I'm pinging you as you originally set the 3.0 milestone.

@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.0.0, 2.17.0 Feb 21, 2023
@Pierre-Sassoulas
Pierre-Sassoulas merged commit d025472 into pylint-dev:main Feb 21, 2023
@alexey-pelykh
alexey-pelykh deleted the feature/source-roots-globbing branch February 22, 2023 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement ✨ Improvement to a component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support globbing patterns for source-roots

5 participants