-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
Backport patch for 6.8.0 and add tests for future failures #249
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
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.
Besides my tiny comment below, I have another question: why not wait for 6.8.1?
recipe/meta.yaml
Outdated
- python -c "from qtpy import API; assert API == 'pyside6', 'PySide6 not in use'" # [py==311] | ||
- python -c "from qtpy.QtCore import Qt" # [py==311] |
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.
Why is this test marked for 3.11 only? I think it be better to leave it for any version.
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.
I don’t like adding too many optional dependencies for these nice to have tests.
By having it only for python 3.11 we test it, while still testing minimal dependencies in other pythons
its hard to "merge red", i could downgrade the version, but it was "just as easy" to try the upstream patch. |
The nice thing is that upstream seems to have added a similar test to the one I added to their automation. |
Ok, I was just curious.
Finally! Haha |
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.
imho it was enough to check the start import, and even that is
recipe/meta.yaml
Outdated
@@ -81,6 +88,8 @@ requirements: | |||
|
|||
test: | |||
requires: | |||
# Add some qtpy tests to ensure we don't fully break things | |||
- qtpy # [py==311] |
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.
checking with qtpy is overkill imho
checking star import would suffice, but even that is already done in the pyside side so there should be no regressions:
pyside/pyside-setup@2a2d013
but its still +1 even if you want to keep it
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.
ok but i might add it back within the year ;)
Hi! This is the friendly conda-forge automerge bot! I considered the following status checks when analyzing this PR:
Thus the PR was passing and merged! Have a great day! |
For some reason the CIs didn't trigger. I pushed directly to the |
I thought that this would be a good test to add, it did have the desired effect, then I tried their patch and it seems ok.