Skip to content
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

Clear warnings for each file in codemod cli #1184

Merged
merged 3 commits into from
Aug 5, 2024
Merged

Clear warnings for each file in codemod cli #1184

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b0dbe99
Clean warnings for each file in comemod cli
kiri11 Aug 3, 2024
4077ff0
Fix ZeroDivisionError: float division by zero
kiri11 Aug 3, 2024
71986bf
Recreate CodemodContext for each file
kiri11 Aug 4, 2024
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
5 changes: 4 additions & 1 deletion libcst/codemod/_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,10 +250,13 @@
# We do this after the fork so that a context that was initialized with
# some defaults before calling parallel_exec_transform_with_prettyprint
# will be updated per-file.
# Clean the warnings as well, otherwise they will be
# passed from the previous file
transformer.context = replace(
transformer.context,
filename=filename,
scratch=deepcopy(scratch),
warnings=[],
Copy link
Member

Choose a reason for hiding this comment

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

While this change will fix the issue, there's a deeper problem here. Apart from metadata_manager, every other field of context should be reset per file.
Concretely, I'm surprised we change filename here but not full_module_name or full_package_name.

Maybe we should make this explicit and have a global context that persists between files, and a local one that gets reset. Then this assignment would look something like:

try:
    mod_and_pkg = calculate_module_and_package(config.repo_root or ".", filename)
    mod_name = mod_and_pkg.name
    pkg_name = mod_and_pkg.package
except ValueError as exc:
    print(f"Failed to determine module name for {filename}: {ex}", file=sys.stderr)
    mod_name = None
    pkg_name = None
    
transformer.context = CodemodContext(scratch=deepcopy(scratch), filename=filename, full_module_name=mod_name, full_package_name=pkg_name, metadata_manager=transformer.context.metadata_manager)

Copy link
Member

Choose a reason for hiding this comment

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

Having said that, I'm happy to merge the PR as is because it's definitely an improvement over the current state. Let me know if you wanna roll this change into this PR or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

full_module_name or full_package_name are changed below, so currently context replaced twice. And if it isn't possible to determine the module/package name, it will persist from the previous file. The wrapper also persists. That would be changing in the new commit I'm submitting.

Before:

  • full_module_name/full_package_name persist if can't get them for the new file
  • wrapper persists across files

After:

  • full_module_name/full_package_name both set to None if can't figure them out.
  • wrapper is set to None on every file.

Is that okay to not set the wrapper?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, it should be

)

# determine the module and package name for this file
Expand Down Expand Up @@ -420,7 +423,7 @@
operations still to do.
"""

if files_finished <= 0:
if files_finished <= 0 or elapsed_seconds == 0:

Check warning on line 426 in libcst/codemod/_cli.py

View check run for this annotation

Codecov / codecov/patch

libcst/codemod/_cli.py#L426 

Added line #L426 was not covered by tests
# Technically infinite but calculating sounds better.
return "[calculating]"

Expand Down
30 changes: 30 additions & 0 deletions libcst/codemod/tests/test_codemod_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@
import platform
import subprocess
import sys
import tempfile
from pathlib import Path
from unittest import skipIf

from libcst._parser.entrypoints import is_native
from libcst.codemod import CodemodTest
from libcst.testing.utils import UnitTest


Expand Down Expand Up @@ -63,3 +65,31 @@ def test_codemod_external(self) -> None:
stderr=subprocess.STDOUT,
)
assert "Finished codemodding 1 files!" in output

def test_warning_messages_several_files(self) -> None:
code = """
def baz() -> str:
return "{}: {}".format(*baz)
"""
with tempfile.TemporaryDirectory() as tmpdir:
p = Path(tmpdir)
(p / "mod1.py").write_text(CodemodTest.make_fixture_data(code))
(p / "mod2.py").write_text(CodemodTest.make_fixture_data(code))
(p / "mod3.py").write_text(CodemodTest.make_fixture_data(code))
output = subprocess.run(
[
sys.executable,
"-m",
"libcst.tool",
"codemod",
"convert_format_to_fstring.ConvertFormatStringCommand",
str(p),
],
encoding="utf-8",
stderr=subprocess.PIPE,
)
# Each module will generate a warning, so we should get 3 warnings in total
self.assertIn(
"- 3 warnings were generated.",
output.stderr,
)
Loading