fix: Update CI configuration for PyPy compatibility #47
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add proper GitHub Actions workflow configuration - Simplify tox configuration - Add explicit PyPy support - Fix dependency installation issues
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to c471156 in 16 seconds
More details
- Looked at
128lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. .github/workflows/ci.yml:28
- Draft comment:
Add a newline at the end of the file for POSIX compliance. - Reason this comment was not posted:
Confidence changes required:10%
The CI configuration is missing a newline at the end of the file, which is a best practice for POSIX compliance.
2. tox.ini:33
- Draft comment:
Add a newline at the end of the file for POSIX compliance. - Reason this comment was not posted:
Confidence changes required:10%
The tox configuration is missing a newline at the end of the file, which is a best practice for POSIX compliance.
Workflow ID: wflow_xB51BbVEFlTVcImN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
- Add matrix strategy for all Python versions - Include all operating systems - Restore check and docs jobs - Fix PyPy configuration
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8835e6a in 37 seconds
More details
- Looked at
85lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/ci.yml:41
- Draft comment:
Setting multiple Python versions in a single job can lead to unexpected behavior. Consider setting only one version per job run. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment appears to misunderstand the setup. This is a standard tox testing setup where multiple Python versions are installed to run the test matrix. The TOXENV environment variable is set from matrix.tox-env, and tox-gh-actions will handle selecting the appropriate Python version for each test run. This is actually the recommended way to set up Python testing with tox in GitHub Actions.
Maybe there could be some overhead from installing multiple Python versions when only one is needed for each specific test run?
The slight overhead of installing multiple versions is outweighed by the benefits of having a single job definition that handles all test combinations, making the workflow more maintainable.
The comment should be deleted as it raises a false concern. The multiple Python version setup is intentional and correct for this tox-based testing workflow.
Workflow ID: wflow_ojlBhtrrlhvNazaF
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
- Map each tox environment to specific Python version - Add proper matrix includes - Improve error reporting with verbose tox output - Fix dependency installation steps
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 0ff8179 in 39 seconds
More details
- Looked at
97lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/ci.yml:34
- Draft comment:
Theincludesection undermatrixis redundant since all the environments are already listed in thetox-envarray. Consider removing theincludesection to simplify the configuration. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests removing the include section because it lists the same environments, but this misses the crucial point that the include section adds the python-version mapping that's needed for the workflow to function. Without this mapping, the workflow wouldn't know which Python version to use for each tox environment. The include section serves an essential purpose.
Could there be a simpler way to map tox environments to Python versions that I'm not seeing? Maybe there's a tox feature that makes this mapping unnecessary?
While there might be other ways to handle the version mapping, the current approach using matrix.include is a standard GitHub Actions pattern and clearly shows the relationship between tox environments and Python versions.
The comment should be deleted because it's incorrect - the include section serves a necessary purpose by mapping tox environments to their corresponding Python versions.
Workflow ID: wflow_qFGHgYdWFAIuZTb8
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on aee1fa4 in 19 seconds
More details
- Looked at
182lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/ci.yml:35
- Draft comment:
The--skip-missing-interpreters falseoption is redundant sinceskip_missing_interpreters = trueis already set intox.ini. Consider removing it for clarity. - Reason this comment was not posted:
Confidence changes required:50%
The--skip-missing-interpreters falseoption in thetoxcommand is redundant because thetox.inifile already hasskip_missing_interpreters = true. This redundancy should be removed for clarity and to avoid confusion.
Workflow ID: wflow_P0okNJTow1SuYN16
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
- Install gettext using Homebrew on macOS runners - Force link gettext to resolve library dependency - Conditional execution only on macOS
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c377c5c in 52 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/ci.yml:27
- Draft comment:
Usingbrew link gettext --forcecan overwrite existing symlinks, which is not ideal. Consider usingbrew link --overwrite gettextinstead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about changed code. While technically correct that '--overwrite' is an alternative, '--force' is a commonly used and valid option. The difference between them is minor, and both would work. The current solution with '--force' is not wrong or problematic.
The comment might be pointing out a real best practice for Homebrew that I'm not aware of. There could be subtle differences between '--force' and '--overwrite' that matter in CI environments.
While there might be subtle differences, the current solution with '--force' is well-established and commonly used. The suggested change doesn't provide significant benefits to justify a code change.
Delete the comment because it suggests an alternative that isn't clearly better than the current valid solution.
Workflow ID: wflow_NSMOfkiTG906uvGN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d069284 in 11 seconds
More details
- Looked at
49lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/ci.yml:24
- Draft comment:
Consider adding a comment or note explaining why PyPy 3.8 support is being removed, as this could impact users relying on that version. This applies to both the CI configuration and the tox environment list. - Reason this comment was not posted:
Confidence changes required:50%
The removal of PyPy 3.8 from the CI configuration and tox environment list is consistent across both files. However, the reason for this removal is not documented in the PR description. It would be beneficial to include a comment or note explaining why PyPy 3.8 support is being dropped, as this could impact users relying on that version.
Workflow ID: wflow_MCjy7HXK9HVgSZUY
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 73bf269 in 1 minute and 13 seconds
More details
- Looked at
558lines of code in23files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/datapilot/core/platforms/dbt/utils.py:265
- Draft comment:
Theget_hard_coded_referencesfunction should identify hard-coded references likeschema.table1, not templated references like{{ var('schema.table') }}. Please ensure the function and its test cases are aligned with this purpose. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_DCNEu28GuNy59F3g
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Update CI configuration for PyPy compatibility and improve code formatting and testing setup.
.github/workflows/ci.ymlfor GitHub Actions to run tests on multiple OS and Python versions, including PyPy 3.9 and 3.10.tox.inito simplify environment definitions and add PyPy support.check_macro_args_have_desc.py,check_macro_has_desc.py, and other files.setup.pyandtest_utils.py.setup.cfgforflake8configuration.This description was created by
for 73bf269. It will automatically update as commits are pushed.