-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
send_super
fixes
#220
send_super
fixes
#220
Conversation
This allows using versions of Python which are not preinstalled in the tools cache on GitHub-hosted runners.
5539642
to
4c72e66
Compare
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.
Patch looks good; 1 question, and 1 addition request.
The question: The updated version on the setup-python action - is that just a case of using the most recent version, or is there something specific being exercised for which we need the upgrade?
The request: Test cases. We've got a test case for incorrect argument lists for send_message
; we should add a similar test set for send_super
.
The smoke test on Python 3.5 was failing because Python 3.5 no longer comes pre-installed on GitHub test runners. setup-python@v2 allows downloading Python versions which are not preinstalled. The alternative would have been to migrate the test to Python 3.6 but I've found that in the past, tests on Python 3.5 have caught some allocation / deallocation related segfaults which appeared to be more flaky in Python > 3.5.
Fair point, will do. |
Having access to a broader range of Python versions will definitely be useful. However, Python 3.5 is now officially EOL, so IMHO we don't need to go to excessive lengths to support it. While it may provide utility in surfacing memory allocation errors, using 3.5 won't be a good strategy for the long term, so if we have concerns, we should find some other way (like valgrind) to surface those errors. |
I agree, there is no strong case to support it. For now, it just seemed painless to keep testing on Python 3.5 as long as we don't want or need to drop support for any code changes. |
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.
👍 Looks good - thanks for the fix!
This PR fixes #219.
It also aims to provide a uniform API between
send_message
andsend_super
. Forsend_super
,argtypes
is a keyword argument which defaults to an empty list. This can be convenient if there are noargs
provided to the call in the first place. I've therefore adapted this forsend_message
as well. Alternatively, one might makeargtypes
a mandatory argument forsend_super
. In any case, I would argue that a uniform API would be nice here.PR Checklist: