-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Change parsing for exclude field #11746
Conversation
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
A couple of questions:
- We have
files
that is similar toexclude
. Is it consistent with your solution? - We can surely test this! Take a look at
mypy/test/test_find_sources.py
'exclude': lambda s: [ | ||
p.strip() for p in | ||
(s.split('\n') if isinstance(s, str) else s) | ||
if p.strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can turn this into a separate function, so we can skip doing some work twice: p.strip()
for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I were to, I'd probably also change the ini exclude
function that I stole it from:
Line 129 in fee25c3
'exclude': lambda s: [p.strip() for p in s.split('\n') if p.strip()], |
I considered using the try_split()
function, but it would not have filtered empty strings, I think:
Lines 44 to 49 in fee25c3
def try_split(v: Union[str, Sequence[str]], split_regex: str = '[,]') -> List[str]: | |
"""Split and trim a str or list of str into a list of str""" | |
if isinstance(v, str): | |
return [p.strip() for p in re.split(split_regex, v)] | |
return [p.strip() for p in v] |
Would this work better?
'exclude': list(filter(None, try_split('[\n]')))
Line 138 in fee25c3
That's actually what caught me in the first place: trying to use
I'm worried this means there's a large testing gap that I feel unqualified to fill. |
No worries, I can help you with tests, if all other things are ok! 👍 |
FYI, I have a competing PR (#11828) in the works to address this (i.e., fix #11825). (Apologies for missing yours.) Are you sure you want to split the TOML string? While not documented, some folks may have come to rely on treating multi-line strings in TOML as a single regex. |
It looked like an easy quality-of-life improvement to provide an additional syntax for specifying multiple regexps for I agree that breaking expected behaviour, even if undocumented, is always a problem for people using the tools. I would prefer to not split TOML strings if there's a nicer, more structured syntax available, but I do prefer parity and unity of behaviours and features between all sources of configuration, first and foremost. Your PR (#11825) does more, and more that I like, so I'll close this one in favour of that one. |
Description
#11329 added support for nicer
exclude
listsTOML 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.Examples:
produces:
All of these produce the same result:
Test Plan
I've only tested on the commandline.