-
Notifications
You must be signed in to change notification settings - Fork 8
feat: fix bugs and add modern python support #2
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
Conversation
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.
Pull Request Overview
This PR modernizes the expression parser by fixing bugs and adding support for Python versions 2.7-3.14. Key improvements include fixing chained comparison logic, implementing proper short-circuit evaluation for boolean operators, and adding modern Python literal support.
- Fix chained comparison evaluation (pairwise with short-circuit) and boolean short-circuit semantics
- Add Python 3.8+
visit_Constantmethod while maintaining backward compatibility - Restrict function calls to direct names only and make ast.parse mode explicit
- Move comprehensive tests to proper test file structure and add GitHub CI workflows
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_expression_parser.py | New comprehensive test file with all expression parser tests including chained comparisons and boolean short-circuit behavior |
| tests/init.py | Simplified to package marker, moving tests to proper test file |
| expression/parser.py | Core fixes for chained comparisons, boolean short-circuit, Python 3.8+ support, and function call restrictions |
| expression/interpreter.py | Code formatting improvements with consistent string quotes |
| expression/init.py | Version bump and consistent string quote formatting |
| setup.py | Formatting improvements with consistent style |
| .github/workflows/lint.yml | New GitHub workflow for code linting with Ruff |
| .github/workflows/ci.yml | New CI workflow with multi-version Python testing (2.7-3.14) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
lhelwerd
left a comment
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.
Hi and thanks for your interest in updating this small module to support newer Python versions. Indeed I have not been maintaining it closely due to not using it in any projects since when these versions (2.7, 3.6) were more commonplace in distros and containerized environments.
As mentioned in the linked issue, there are likely alternatives that would work in your use case just as well (or even better, such as limiting the operands of the power operations to some maximum value). So going ahead to submit a PR is a nice thing (even if ai-ded).
Overall, the changes are fine; good to have some code formatting and a working CI (I might actually feel inspired to modernize as well, replace the build toolchain from setup.py to pyproject.toml and add a proper changelog) and thanks for the tests for the short-circuiting/chaining.
I left a few comments for cleanup, could you have a look?
After changes and approval, I can make a new release with the newer python support. Later on, 2.7 and the unsupported python3 versions could be dropped in a major version bump (I guess it's been long enough).
d38f6b9 to
a9e40a2
Compare
- Implement true short-circuit for 'and'/'or' returning deciding operand
- Fix chained comparisons to evaluate pairwise with short-circuit
- Restrict function calls to direct names only (reject attribute/lambda)
- Add visit_Constant for Python 3.8+ unified literals
(keep visit_Num/visit_NameConstant for legacy)
- Make ast.parse mode explicit ('exec') for assignment compatibility
- Remove obsolete _boolean_ops mapping
tests: add coverage and move to proper file
- Add test_expression_parser.py (moved from tests/__init__.py)
- Add: test_chained_comparisons
- Add: test_boolean_short_circuit
- Add: test_call_requires_direct_function_name
- Add: test_starstar_kwargs_not_supported
- Add: test_attribute_access_disallowed
- Add: test_parenthesized_function_call_allowed
ci: introduce GitHub Actions
- Matrix for Python 3.8–3.14 and separate Python 2.7 job via container
- Separate non-blocking ruff lint workflow using Astral actions
chore(gitignore): sync with latest GitHub Python template
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.
Pull Request Overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Your package is still powering the Fishtest workers. Consider this PR a small token of appreciation - feel free to merge it whenever it's convenient for you. I’ve opened a PR, currently on hold because it requires python >= 3.8, to switch to |
|
Thank you for the fast release of the new version, I updated Fishtest worker. A couple of our users are running high core count workers with python 3.6, your package keeps them up and running. |
feat(parser): add modern python support
(keep visit_Num/visit_NameConstant for legacy)
tests: add coverage and move to proper file
ci: introduce GitHub Actions
chore(gitignore): sync with latest GitHub Python template