Repository navigation
[codex] improve package-mode compression - #13
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53d996504c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _is_active_parameter_name(self, name): | ||
| if self.rename_arguments: | ||
| return False |
There was a problem hiding this comment.
Returning early from _is_active_parameter_name whenever rename_arguments is true makes assignment targets for unrenamed parameters look like ordinary locals, so _rename_assignment_target can create a new mapping and break control-flow semantics. For example, def f(x, flag):\n if flag: x = 2\n return x is transformed so the return reads a different name that is only conditionally assigned, raising UnboundLocalError when flag is false.
Useful? React with 👍 / 👎.
| elif base_name in self.class_member_mappings: | ||
| attribute_mapping = self.class_member_mappings[base_name] |
There was a problem hiding this comment.
Attribute remapping now only consults class names and receiver names, so calls through local instance variables are no longer updated after method renaming. With default keep_global_variables=False, a class method like test can be renamed in the class body while instance.test(...) is left unchanged, which fails at runtime with AttributeError because the renamed class no longer has test.
Useful? React with 👍 / 👎.
| base_name = func.value.id if isinstance(func.value, ast.Name) else None | ||
| if base_name is None: | ||
| return None |
There was a problem hiding this comment.
_call_argument_info now returns metadata only when func.value is an ast.Name, so calls like C().method(long_value=1) have no argument rewrite info. When rename_arguments=True renames method parameters, these constructor-call keywords stay on the old names and raise TypeError (unexpected keyword argument) at runtime.
Useful? React with 👍 / 👎.
| if imported_name != alias.name and imported_name in self.callable_argument_infos: | ||
| self.callable_argument_infos[alias.name] = self.callable_argument_infos.pop(imported_name) |
There was a problem hiding this comment.
When an imported symbol is renamed and also has an as alias, this block moves callable argument metadata from the local alias name to the imported symbol name. That loses rewrite metadata for actual call sites (which use the alias), so from main import square as g followed by g(long_value=1) is left unrevised and fails once square's parameter is renamed.
Useful? React with 👍 / 👎.
Summary
This PR pushes package-mode compression further while keeping the validated TexSoup package path working.
--rename-argumentsto the CLI and APIself/clsand rewrite safe internal keyword call sitesWhy
The current package-mode compression still left easy wins on the table:
self/cls, were still left long even under the aggressive settingsThis keeps bundle mode unchanged and focuses on package mode, which is the validated path for TexSoup.
Impact
Current measured results with
--rename-modules --rename-global-variables --rename-arguments:tests/examples/pyminifier.py:1,355 -> 438bytes (67.7%)tests/examples/pyminify.py:1,990 -> 935bytes (53.0%)TexSoup/*.py:98,181 -> 24,722bytes (74.8%)TexSoup.tar.gz:23,656 -> 9,208bytes (61.1%)Validation
PYTHONPATH=. .venv/bin/python -m pytestPYTHONPATH=. .venv/bin/python scripts/regenerate_examples.py --checkPYTHONPATH=. .venv/bin/python -m pymini package /tmp/pymini-texsoup-repo/TexSoup -o /tmp/pymini-texsoup-pass/TexSoup --rename-modules --rename-global-variables --rename-argumentsPYTHONPATH=/tmp/pymini-texsoup-pass:/tmp/pymini-texsoup-repo/tests .venv/bin/python -m pytest /tmp/pymini-texsoup-repo/tests -o addopts=''Upstream TexSoup still passes with
78tests green.