Support --exclude more than once on command line #11329

nipunn1313 · 2021-10-13T22:03:31Z

Description

Support --exclude more than once on command line

The result is an "OR" of all the patterns provided.
Should be fully backward compatible to existing folks.

Test Plan

Added tests for the new behavior to test_find_sources.py. Also did a quick manual test to make sure command line arguments are working ok.

hauntsaninja

Thanks for working on this, maybe config is parsing it as a string still?

nipunn1313 · 2021-10-14T21:55:59Z

Yep - when I add it to setup.cfg, it's parsing as (using ibis as an example)

[mypy]
exclude = versioneer.py

as

['v', 'e', 'r', 's', 'i', 'o', 'n', 'e', 'e', 'r', '.', 'p', 'y']

I added four integration tests that should help cover this case and fixed the issue.
Good on mypy_primer!!

nipunn1313 · 2021-10-14T21:58:05Z

docs/source/config_file.rst

@@ -197,9 +197,9 @@ section of the command line docs.

 .. confval:: exclude

-    :type: regular expression
+    :type: comma-separated list of regular expressions


this actually seems bad - because commas can appear in regular expressions. Perhaps we need another way to delimit multiple entries? Is there precedent?

Per #10310 (comment) - I opted to go for newline separated list of regexes and updated the docs

SS of updated docs

The result is an "OR" of all the patterns provided. Should be fully backward compatible to existing folks. Fixes python#10310

nipunn1313 · 2021-10-14T22:20:09Z

Update command_line.rst doc screenshot

nipunn1313 · 2021-10-22T02:28:27Z

Hiya - wanted to check in w/ the maintainers on this one again (no rush - just want to make sure it wasn't forgotten)

hauntsaninja · 2021-10-22T05:14:27Z

Thanks, this is great!

cclauss · 2021-11-05T07:41:13Z

It would be awesome to get a release that contains this fix.

python#11329 added support for nicer `exclude` lists TOML has a built-in list syntax, and it would be nice to use that for specifying lists for the `exclude` option. This change tries the ini-style first: if `exclude` is set to a multiline string, it will split that on newlines, otherwise it will assume it's a list.

callahad · 2021-12-16T20:00:50Z

For anyone following along at home, this does not appear to have made it into today's release of mypy 0.920.

posita · 2021-12-22T21:40:42Z

How do these get represented in pyproject.toml? Could this be related to #11825?

posita · 2021-12-22T21:45:10Z

mypy/config_parser.py

@@ -124,6 +124,7 @@ def check_follow_imports(choice: str) -> str:
    'cache_dir': expand_path,
    'python_executable': expand_path,
    'strict': bool,
+    'exclude': lambda s: [p.strip() for p in s.split('\n') if p.strip()],


Possible root cause for #11825?

Yes - seems likely. Good find. The config_parser uses very basic newlines to split apart different excludes.

The behavior is documented in the config_file.rst - about multiple patterns in a newline separated manner.

Didn't realize that multiline excludes like this were an old pattern
Suggestion on what to do to fix?

Didn't realize that multiline excludes like this were an old pattern

To be fair, I don't know if that was ever official. It was more of a work-around that folks discovered because they (probably like you) were desperately seeking a fix for #10310.

config_parser.py is new to me, and I'm still trying to digest it. At a high level, I'm wondering if one can treat .toml files differently? It looks like there's a mechanism to do that, but I'm still trying to figure out if it's sufficient for what I have in mind, i.e., in a .toml file, if it's a string, just parse it as a single regex, but if it's an array, treat each item as its own regex? (Just thinking out loud here.)

~~Looking further, I think this might be as simple as appending the following here~~:

toml_config_types.update({ # … 'exclude': try_split, })

UPDATE: Nope. No. That would still try to split strings along commas.

Dunno what testing looks like yet, though….

If I know you, you're probably ¾ done with a PR by now, but if you haven't started, I can take a stab….

I haven't started. Go for it!
Appreciate the compliment :)

I haven't looked at the details, but there's also this PR that was opened recently: #11746

Thanks for the heads-up @hauntsaninja! I don't think that one works. I'll comment there.

#11828 is up. I commented on #11746.

Partially reverts python#11329 (with respect to `.ini` documentation). Modifies existing unit tests. Fixes python#11830.

Partially reverts #11329 (with respect to `.ini` documentation). Modifies existing unit tests. Fixes #11830. Co-authored-by: Matthew W <matthew@willcockson.family>

The result is an "OR" of all the patterns provided. Should be fully backward compatible to existing folks. Fixes python#10310

Partially reverts python#11329 (with respect to `.ini` documentation). Modifies existing unit tests. Fixes python#11830. Co-authored-by: Matthew W <matthew@willcockson.family>

nipunn1313 mentioned this pull request Oct 13, 2021

exclude key in mypy.ini as a single regex doesn't scale for larger code bases #10310

Closed

This comment has been minimized.

Sign in to view

hauntsaninja reviewed Oct 14, 2021

View reviewed changes

nipunn1313 force-pushed the multiple_exclude branch from dcc7f3b to 3083adc Compare October 14, 2021 21:52

nipunn1313 commented Oct 14, 2021

View reviewed changes

Support --exclude more than once on command line

4dab5a7

The result is an "OR" of all the patterns provided. Should be fully backward compatible to existing folks. Fixes python#10310

nipunn1313 force-pushed the multiple_exclude branch from 3083adc to 4dab5a7 Compare October 14, 2021 22:18

hauntsaninja merged commit 01d6eb8 into python:master Oct 22, 2021

nipunn1313 deleted the multiple_exclude branch October 30, 2021 18:01

callahad mentioned this pull request Nov 8, 2021

Get synapse/ to pass mypy matrix-org/synapse#11271

Open

mawillcockson mentioned this pull request Dec 15, 2021

Change parsing for exclude field #11746

Closed

posita mentioned this pull request Dec 22, 2021

Crash/Regression - 0.930 no longer accepts multiline basic strings for exclude patterns in pyproject.toml #11825

Closed

posita reviewed Dec 22, 2021

View reviewed changes

posita mentioned this pull request Dec 22, 2021

Exclude multiple directories on mypy commandline: improve documentation #10250

Closed

asottile mentioned this pull request Dec 23, 2021

handling of exclude configuration changed in an incompatible way in 0.930 #11830

Closed

hauntsaninja mentioned this pull request Dec 29, 2021

Release 0.930 planning #11700

Closed

posita added a commit to posita/mypy that referenced this pull request Jan 1, 2022

Revert to treating exclude in .ini as single string

1d010d0

Partially reverts python#11329 (with respect to `.ini` documentation). Modifies existing unit tests. Fixes python#11830.

posita mentioned this pull request Jan 1, 2022

Revert to treating exclude in .ini as single string #11881

Merged

matrixbot mentioned this pull request Dec 21, 2023

Get synapse/ to pass mypy element-hq/synapse#11271

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support --exclude more than once on command line #11329

Support --exclude more than once on command line #11329

nipunn1313 commented Oct 13, 2021

This comment has been minimized.

hauntsaninja left a comment •

edited

nipunn1313 commented Oct 14, 2021

nipunn1313 Oct 14, 2021

nipunn1313 Oct 14, 2021

nipunn1313 commented Oct 14, 2021

nipunn1313 commented Oct 22, 2021

hauntsaninja commented Oct 22, 2021

cclauss commented Nov 5, 2021

callahad commented Dec 16, 2021

posita commented Dec 22, 2021

posita Dec 22, 2021

nipunn1313 Dec 22, 2021

nipunn1313 Dec 22, 2021

posita Dec 22, 2021 •

edited

posita Dec 22, 2021 •

edited

posita Dec 22, 2021

nipunn1313 Dec 22, 2021

hauntsaninja Dec 22, 2021

posita Dec 22, 2021

posita Dec 22, 2021

Support --exclude more than once on command line #11329

Support --exclude more than once on command line #11329

Conversation

nipunn1313 commented Oct 13, 2021

Description

Test Plan

This comment has been minimized.

hauntsaninja left a comment • edited

Choose a reason for hiding this comment

nipunn1313 commented Oct 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nipunn1313 commented Oct 14, 2021

nipunn1313 commented Oct 22, 2021

hauntsaninja commented Oct 22, 2021

cclauss commented Nov 5, 2021

callahad commented Dec 16, 2021

posita commented Dec 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

posita Dec 22, 2021 • edited

Choose a reason for hiding this comment

posita Dec 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hauntsaninja left a comment •

edited

posita Dec 22, 2021 •

edited

posita Dec 22, 2021 •

edited