Skip to content

Clone dev 11171 #11

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+188 −0
Open

Clone dev 11171 #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec96941
Fix config parsing to allow trailing commas but prevent empty entries
x612skm Jan 15, 2025
0820752
fix the type check error in the pipeline checks
x612skm Feb 24, 2025
e80c2fa
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions mypy/config_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,9 @@ def split_and_match_files_list(paths: Sequence[str]) -> list[str]:
expanded_paths = []

for path in paths:
if not path:
continue

path = expand_path(path.strip())
Comment on lines +112 to 115
Copy link

Choose a reason for hiding this comment

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

medium

This check for an empty path is good, but it doesn't account for paths that consist only of whitespace. A path like ' ' will pass the if not path: check, but path.strip() on the next line will turn it into an empty string, which will then be processed and likely added to expanded_paths.

To handle this correctly, you should strip the path before checking if it's empty.

Suggested change
if not path:
continue
path = expand_path(path.strip())
path = path.strip()
if not path:
continue
path = expand_path(path)

globbed_files = fileglob.glob(path, recursive=True)
if globbed_files:
Expand Down Expand Up @@ -318,6 +321,23 @@ def parse_config_file(
print(f"{file_read}: No [mypy] section in config file", file=stderr)
else:
section = parser["mypy"]

if "files" in section:
raw_files = section["files"].strip()
files_split = [file.strip() for file in raw_files.split(",")]

# Remove trailing empty entry if present
if files_split and files_split[-1] == "":
files_split.pop()

# Raise an error if there are any remaining empty strings
if "" in files_split:
raise ValueError(
"Invalid config: Empty filenames are not allowed except for trailing commas."
)
Comment on lines +334 to +337
Copy link

Choose a reason for hiding this comment

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

Uninformative Error Message

The error message doesn't provide specific information about which entries are empty or their positions in the input. This violates the repository guideline that error messages should be more informative to help users debug configuration issues.

Standards
  • Repo-Guideline-The error message could be more informative


options.files = files_split
Comment on lines +324 to +339
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Bypasses existing file processing logic.

This custom parsing bypasses the registered ini_config_types["files"] handler at line 177, which invokes split_and_match_files. That function performs essential operations:

  • Path expansion (expand_path handles ~ and environment variables)
  • Glob pattern matching (supports *.py, **/*.py, etc.)
  • Recursive directory traversal

Without these, users cannot use globs or environment variables in INI files, creating an inconsistency with TOML (line 207 uses split_and_match_files_list).

Remove this custom parsing and let parse_section (line 342) apply the registered handler:

-        if "files" in section:
-            raw_files = section["files"].strip()
-            files_split = [file.strip() for file in raw_files.split(",")]
-
-            # Remove trailing empty entry if present
-            if files_split and files_split[-1] == "":
-                files_split.pop()
-
-            # Raise an error if there are any remaining empty strings
-            if "" in files_split:
-                raise ValueError(
-                    "Invalid config: Empty filenames are not allowed except for trailing commas."
-                )
-
-            options.files = files_split
-
         prefix = f"{file_read}: [mypy]: "

If empty-string validation is needed, add it to split_and_match_files (line 125) or split_and_match_files_list (line 102) where the guard at lines 112-113 already skips empty paths.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if "files" in section:
raw_files = section["files"].strip()
files_split = [file.strip() for file in raw_files.split(",")]
# Remove trailing empty entry if present
if files_split and files_split[-1] == "":
files_split.pop()
# Raise an error if there are any remaining empty strings
if "" in files_split:
raise ValueError(
"Invalid config: Empty filenames are not allowed except for trailing commas."
)
options.files = files_split
prefix = f"{file_read}: [mypy]: "
🤖 Prompt for AI Agents 
In mypy/config_parser.py around lines 324 to 339, the current custom parsing of
the "files" option bypasses the registered ini_config_types["files"] handler
(defined near line 177) and its split_and_match_files logic (around line 125),
which provides path expansion, glob matching, and recursion; remove this custom
parsing block so parse_section (around line 342) will invoke the registered
handler instead, and if you still require empty-string validation move that
guard into split_and_match_files or split_and_match_files_list (around lines
102–125) where empty paths are already handled.

Comment on lines +325 to +339
Copy link

Choose a reason for hiding this comment

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

Inconsistent Validation Pattern

The code performs partial cleanup before validation by removing trailing empty entries, then validates the remaining input. This violates the repository guideline to validate complete input consistently rather than special-casing positions like trailing elements.

Standards
  • Repo-Guideline-Avoid partial cleanup before validation; validate complete input consistently rather than special-casing positions like trailing elements.


prefix = f"{file_read}: [mypy]: "
updates, report_dirs = parse_section(
prefix, options, set_strict_flags, section, config_types, stderr
Expand Down
168 changes: 168 additions & 0 deletions mypy/test/testconfigparser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
import os
import tempfile
from unittest import TestCase, main

from mypy.config_parser import parse_config_file
from mypy.options import Options


class TestConfigParser(TestCase):
def test_parse_config_file_with_single_file(self) -> None:
"""A single file should be correctly parsed."""
with tempfile.TemporaryDirectory() as tmpdirname:
config_path = os.path.join(tmpdirname, "test_config.ini")

with open(config_path, "w") as f:
f.write(
"""
[mypy]
files = file1.py
"""
)

options = Options()

parse_config_file(options, lambda: None, config_path, stdout=None, stderr=None)

self.assertEqual(options.files, ["file1.py"])

def test_parse_config_file_with_no_spaces(self) -> None:
"""Files listed without spaces should be correctly parsed."""
with tempfile.TemporaryDirectory() as tmpdirname:
config_path = os.path.join(tmpdirname, "test_config.ini")

with open(config_path, "w") as f:
f.write(
"""
[mypy]
files =file1.py,file2.py,file3.py
"""
)

options = Options()

parse_config_file(options, lambda: None, config_path, stdout=None, stderr=None)

self.assertEqual(options.files, ["file1.py", "file2.py", "file3.py"])

def test_parse_config_file_with_extra_spaces(self) -> None:
"""Files with extra spaces should be correctly parsed."""
with tempfile.TemporaryDirectory() as tmpdirname:
config_path = os.path.join(tmpdirname, "test_config.ini")

with open(config_path, "w") as f:
f.write(
"""
[mypy]
files = file1.py , file2.py , file3.py
"""
)

options = Options()

parse_config_file(options, lambda: None, config_path, stdout=None, stderr=None)

self.assertEqual(options.files, ["file1.py", "file2.py", "file3.py"])

def test_parse_config_file_with_empty_files_key(self) -> None:
"""An empty files key should result in an empty list."""
with tempfile.TemporaryDirectory() as tmpdirname:
config_path = os.path.join(tmpdirname, "test_config.ini")

with open(config_path, "w") as f:
f.write(
"""
[mypy]
files =
"""
)

options = Options()

parse_config_file(options, lambda: None, config_path, stdout=None, stderr=None)

self.assertEqual(options.files, [])

def test_parse_config_file_with_only_comma(self) -> None:
"""A files key with only a comma should raise an error."""
with tempfile.TemporaryDirectory() as tmpdirname:
config_path = os.path.join(tmpdirname, "test_config.ini")

with open(config_path, "w") as f:
f.write(
"""
[mypy]
files = ,
"""
)

options = Options()

with self.assertRaises(ValueError) as cm:
parse_config_file(options, lambda: None, config_path, stdout=None, stderr=None)

self.assertIn("Invalid config", str(cm.exception))

def test_parse_config_file_with_only_whitespace(self) -> None:
"""A files key with only whitespace should result in an empty list."""
with tempfile.TemporaryDirectory() as tmpdirname:
config_path = os.path.join(tmpdirname, "test_config.ini")

with open(config_path, "w") as f:
f.write(
"""
[mypy]
files =
Copy link

Choose a reason for hiding this comment

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

medium

This test is intended to check a files key with only whitespace, but the value provided is empty. To properly test this case, you should add some whitespace after files =.

Suggested change
files =
files =

"""
)

options = Options()

parse_config_file(options, lambda: None, config_path, stdout=None, stderr=None)

self.assertEqual(options.files, [])

def test_parse_config_file_with_mixed_valid_and_invalid_entries(self) -> None:
"""Mix of valid and invalid filenames should raise an error."""
with tempfile.TemporaryDirectory() as tmpdirname:
config_path = os.path.join(tmpdirname, "test_config.ini")

with open(config_path, "w") as f:
f.write(
"""
[mypy]
files = file1.py, , , file2.py
"""
)

options = Options()

with self.assertRaises(ValueError) as cm:
parse_config_file(options, lambda: None, config_path, stdout=None, stderr=None)

self.assertIn("Invalid config", str(cm.exception))

def test_parse_config_file_with_newlines_between_files(self) -> None:
"""Newlines between file entries should be correctly handled."""
with tempfile.TemporaryDirectory() as tmpdirname:
config_path = os.path.join(tmpdirname, "test_config.ini")

with open(config_path, "w") as f:
f.write(
"""
[mypy]
files = file1.py,
file2.py,
file3.py
"""
)

options = Options()

parse_config_file(options, lambda: None, config_path, stdout=None, stderr=None)

self.assertEqual(options.files, ["file1.py", "file2.py", "file3.py"])
Comment on lines +9 to +164
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Test coverage gaps: Missing glob and path expansion validation.

These tests validate the custom parsing at mypy/config_parser.py lines 324-339, but that implementation bypasses the existing split_and_match_files handler. If the parsing is corrected to use the registered handler (as recommended), add tests for:

  • Glob patterns (*.py, **/*.py)
  • Home directory expansion (~/myproject/*.py)
  • Environment variable expansion ($HOME/project/*.py)
  • Recursive directory matching

Additionally, test_parse_config_file_with_empty_files_key (lines 67-84) and test_parse_config_file_with_only_whitespace (lines 106-123) appear to be duplicates—both write files = and expect []. Consider removing one.

🤖 Prompt for AI Agents 
In mypy/test/testconfigparser.py around lines 9 to 164, the tests only cover
simple comma/space parsing and duplicate the empty-files case (lines ~67-84 and
~106-123); update the parser to use the existing split_and_match_files handler
(the code path around mypy/config_parser.py lines 324-339 should call that
handler rather than doing custom splitting) and remove the duplicate empty-files
test, then add focused unit tests that verify glob expansion (*.py and **/*.py),
home-directory expansion (~/...), environment-variable expansion ($HOME/...),
and recursive directory matching by creating temporary directories/files and
setting os.environ['HOME'] where needed, invoking parse_config_file, and
asserting options.files contains the resolved file paths (and that invalid/mixed
empty entries still raise ValueError).



if __name__ == "__main__":
main()