-
Notifications
You must be signed in to change notification settings - Fork 131
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
CI fixes because other packages break other packages #1233
Conversation
dask-expr library is now needed.
plotly/Kaleido#226 is the issue. We just need to wait.
Since not worth fixing.
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.
❌ Changes requested. Reviewed everything up to afd93d3 in 42 seconds
More details
- Looked at
61
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. .ci/test.sh:31
- Draft comment:
Consider using a more explicit version check for clarity:
if python -c 'import sys; exit(0) if sys.version_info >= (3, 9) else exit(1)'; then
- Reason this comment was not posted:
Confidence changes required:50%
The PR adds a version check for Python 3.9+ in the test script and test cases, which is a good practice to ensure compatibility. However, the version check in the test script uses a comparison that might not be intuitive at first glance. It would be clearer to use a more explicit comparison for readability.
Workflow ID: wflow_eOExZTSDVU7JndPF
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
This will get easier if we drop 3.8 support, right? It's EOL officially now.
Fixes for CI.
Contains 1 temporary fix around plotly.
Changes
How I tested this
Notes
Checklist