glob patterns should not be overriden by specific --per-file-ignores rules #670
Comments
|
In GitLab by @asottile on Jan 30, 2019, 07:30 It already works for me (?) do you have a particular repository this isn't working in? |
|
In GitLab by @qxv0 on Jan 30, 2019, 08:05 I checked and it seems to me that the problem occurs only in those files which also have another specific ignore rule. For example if I have both |
|
In GitLab by @qxv0 on Jan 30, 2019, 08:08 changed title from {-Allow glob patterns in paths of --per-file-ignor-}es to {+glob patterns should not be overriden by specific --per-file-ignores rul+}es |
|
In GitLab by @qxv0 on Jan 30, 2019, 08:08 changed the description |
|
In GitLab by @asottile on Jan 30, 2019, 08:16 hmmm so the way it's implemented is the first rule that matches a filename is used. In your case you can write: |
|
In GitLab by @oseiberts11 on Feb 6, 2019, 02:12 I seem to have a similar problem (3.7.5). In my per-file-ignores I have (among other things but in this order): First of all, this particular glob pattern doesn't seem to work any more. Even if I expand this to then I still get D* errors for sshca.py: Copying |
|
In GitLab by @sigmavirus24 on Feb 6, 2019, 04:57 @oseiberts11 so to be clear, it sounds like what you expect to happen is that To ignore those three errors in every file matching that glob and then you seem to want to extend the list of ignores for those files for specific files, e.g., |
|
In GitLab by @oseiberts11 on Feb 6, 2019, 07:26 Exactly. That is how it seemed to work with the plugin. (I didn't write the configuration myself, I just noticed that behaviour seems to have changed). |
|
In GitLab by @sigmavirus24 on Feb 6, 2019, 17:26 Right, the plugin took a different tact, honestly, that would be harder to replicate here. Basically, it iterated through everything for matches and applied all of them. We could do that, but then we're favoring exact backwards compatibility (which wasn't ever the goal) over simplicity. It would also mean that we're iterative over a list every time rather than thinking through things more simply, and it makes debugging the option harder in the future, i.e., we always prefer the most specific rule there even if multiple might match. |
|
In GitLab by @oseiberts11 on Feb 7, 2019, 01:11 I suspected something like that. I see a potential solution for people who do want exact backwards compatibility. Maybe you could make it so that if the I realise that this is pretty much the opposite of how the plugin is handled now, but does that sound doable? |
|
In GitLab by @sigmavirus24 on Feb 9, 2019, 05:46 The plugin's page says it's deprecated, I'd rather not encourage folks to use bit-rotting software. Is it too arduous to |
|
In GitLab by @oseiberts11 on Feb 15, 2019, 02:57 For this example with only 2 lines, that would work well enough. But in practice I have several patterns, and several files with additional ignores. From what I've seen above, I understand that the way you want it, is to select a single line, without scanning several or all of them. I fear that in the general case this isn't possible. If there are several wildard patterns that match a particular file name, which one are you going to choose? Why? And in the general case, you cannot just point to "the best pattern" without examining several of them. To give a simple example, with patterns like and file names like Since trying to explain to a user how this works is too complicated (and I'm not even talking about implementing it), just scanning all patterns and applying all matches is much easer to understand, and much easier to implement. (There seems to be a bug in the implementation already, viz. that the |
|
In GitLab by @asottile on Feb 15, 2019, 03:48 I don't think it's hard to explain, the first matching pattern wins right? |
|
In GitLab by @asottile on Feb 15, 2019, 03:57 Also regarding the |
|
In GitLab by @sigmavirus24 on Feb 15, 2019, 04:15 It's the last match sorted alphabetically So it's always consistent and reproducible. If we wanted to throw people under the bus and give them a foot-gun at the same time, we could pretty easily just apply every thing, but then things they didn't intend to match end up hiding problems from them they didn't want. This is precisely why I had no original intention to support globbing. It introduces far too much unnecessary complexity and no one will be happy no matter how it's implemented. |
|
In GitLab by @bob.hyman on May 2, 2020, 16:34 First let's document this, to help save the next developer a day lost to learning what to expect. Here's a draft, please comment: (In --per-file-ignores):
Is this correct? |
…as reported with `lint-vetting` (#792) Disable warnings related to the use of assert in the tests directory as reported with `lint-vetting` This adds a few additional files to the per-file-ignore list within the flake8 configuration with an exemption for S101 which is reported by flake8-bandit as part of the wemake style guide which is being vetted. The list was also alphabetized to ensure new entries were placed in a sane location. A note was added to the top of the flake8 per-file-ignores indicating that the glob for the tests folder ignoring s101 can be added later. Currently there is a conflict between globs and per-file entries. See PyCQA/flake8#670 This eliminates the S101 entry from the output of tox -e lint-vetting so users of it can see messages only related to the changes they are making and not for files that were previously added to the repository: 1 C812 missing trailing comma 1 RST304 Unknown interpreted text role "mod". - 13 S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. 1 S404 Consider possible security implications associated with the subprocess module. 1 S603 subprocess call - check for execution of untrusted input. 2 S604 Function call with shell=True parameter identified, possible security issue. 2 WPS111 Found too short name: p < 2 2 WPS115 Found upper-case constant in a class: KEGEX 3 WPS201 Found module with too many imports: 13 > 12 6 WPS210 Found too many local variables: 6 > 5 3 WPS220 Found too deep nesting: 24 > 20 2 WPS221 Found line with high Jones Complexity: 16 > 14 3 WPS226 Found string literal over-use: test_option > 3 9 WPS305 Found `f` string 1 WPS316 Found context manager with too many assignments 6 WPS317 Found incorrect multi-line parameters 2 WPS323 Found `%` string formatting 4 WPS324 Found inconsistent `return` statement 2 WPS414 Found incorrect unpacking target 1 WPS430 Found nested function: getcwd 14 WPS436 Found protected module import: _curses 3 WPS450 Found protected object import: _CursesWindow 1 WPS602 Found using `@staticmethod` Related #713 Reviewed-by: Sviatoslav Sydorenko <webknjaz+github/profile@redhat.com> Reviewed-by: Bradley A. Thornton <bthornto@redhat.com> Reviewed-by: None <None>
…as reported with `lint-vetting` (ansible#792) Disable warnings related to the use of assert in the tests directory as reported with `lint-vetting` This adds a few additional files to the per-file-ignore list within the flake8 configuration with an exemption for S101 which is reported by flake8-bandit as part of the wemake style guide which is being vetted. The list was also alphabetized to ensure new entries were placed in a sane location. A note was added to the top of the flake8 per-file-ignores indicating that the glob for the tests folder ignoring s101 can be added later. Currently there is a conflict between globs and per-file entries. See PyCQA/flake8#670 This eliminates the S101 entry from the output of tox -e lint-vetting so users of it can see messages only related to the changes they are making and not for files that were previously added to the repository: 1 C812 missing trailing comma 1 RST304 Unknown interpreted text role "mod". - 13 S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. 1 S404 Consider possible security implications associated with the subprocess module. 1 S603 subprocess call - check for execution of untrusted input. 2 S604 Function call with shell=True parameter identified, possible security issue. 2 WPS111 Found too short name: p < 2 2 WPS115 Found upper-case constant in a class: KEGEX 3 WPS201 Found module with too many imports: 13 > 12 6 WPS210 Found too many local variables: 6 > 5 3 WPS220 Found too deep nesting: 24 > 20 2 WPS221 Found line with high Jones Complexity: 16 > 14 3 WPS226 Found string literal over-use: test_option > 3 9 WPS305 Found `f` string 1 WPS316 Found context manager with too many assignments 6 WPS317 Found incorrect multi-line parameters 2 WPS323 Found `%` string formatting 4 WPS324 Found inconsistent `return` statement 2 WPS414 Found incorrect unpacking target 1 WPS430 Found nested function: getcwd 14 WPS436 Found protected module import: _curses 3 WPS450 Found protected object import: _CursesWindow 1 WPS602 Found using `@staticmethod` Related ansible#713 Reviewed-by: Sviatoslav Sydorenko <webknjaz+github/profile@redhat.com> Reviewed-by: Bradley A. Thornton <bthornto@redhat.com> Reviewed-by: None <None>
|
I'd really like to see merging of the excludes from multiple patterns that match. Currently I have this: so in any unit test files I want to disable E241, and in any models I want to disable N805 (due to sqlalchemy hybrid property expression classmethods). Guess where one of the tests that need E241 disabled is... |
|
Quoting from #670 (comment):
I'd also like to see this #670 (comment) too in the documentation:
I didn't loose a day, but a good two hours for sure. Especially given that the first result in |
In GitLab by @qxv0 on Jan 30, 2019, 06:59
flake8 --bug-report:Say we want to ignore D102 errors in all files within thesee the comments belowtestsdirectory. We can add a rule for each file individually, but it would be more efficient if we could do that just by addingtests/* : N813to the config file (similar to the way flake8-per-file-ignores plugin works onflake8<3.7.0).The text was updated successfully, but these errors were encountered: