Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6d003c8a7
ℹ️ 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".
| insert_at = 1 if body and ast.get_docstring(ast.Module(body=body, type_ignores=[])) is not None else 0 | ||
| return body[:insert_at] + assignments + body[insert_at:] |
There was a problem hiding this comment.
Preserve future imports at file start when hoisting
The module/class string-hoisting insertion point only checks for a docstring, so generated assignments can be placed before from __future__ import .... In modules that use future imports and have a repeated hoisted string, the minified output becomes syntactically invalid (SyntaxError: from __future__ imports must occur at the beginning of the file). This is a runtime-breaking regression introduced by module-scope hoisting.
Useful? React with 👍 / 👎.
| rewritten.append(statement) | ||
| continue | ||
| rewritten_statement = RepeatedNameRewriter(mapping).visit(statement) | ||
| rewritten.append(self._alias_assignment(mapping)) |
There was a problem hiding this comment.
Avoid eager name evaluation in repeated-name aliasing
This pass prepends an alias assignment before the original statement, which eagerly loads every repeated name up front. That changes semantics for guarded/short-circuited expressions (for example, condition and missing_name), where a name may never be evaluated in the original code but now raises NameError during alias setup. The optimization should not force earlier evaluation of names used inside the statement.
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.
Restrict class-member rewrites after class-name rebinding
Attribute rewriting keys off bare identifier text in class_member_mappings, so any later Name.attr with the same identifier is rewritten even if that name has been rebound to a different object. A common case is rebinding a class symbol (Foo = type(...)) and then accessing Foo.some_method; this can be rewritten to the shortened attribute and fail with AttributeError on the new object.
Useful? React with 👍 / 👎.
What changed
This restores the remaining optimization passes in
pyminiwith size-aware guardrails instead of all-or-nothing behavior.pyminifyexample output and benchmark numbers in the READMEWhy
The remaining deferred optimizations were worth restoring, but the naive versions regressed small public APIs and package-shaped inputs. The new heuristics keep the wins for larger code while skipping unprofitable rewrites on small fixtures.
Impact
tests/examples/pyminifier.pyreturns to511bytes (62.3%reduction)tests/examples/pyminify.pyis981bytes (50.7%reduction)TexSoupstill passes its upstream test suiteTexSoupraw source is66.3%smaller and the package.tar.gzsnapshot is83.2%smallerValidation
.venv/bin/python -m pytest.venv/bin/python scripts/regenerate_examples.py --checkPYTHONPATH=/tmp/texsoup-restore2.L4TojF .venv/bin/python -m pytest /tmp/pymini-texsoup-repo/tests -o addopts=''