Skip to content
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

Merged
merged 6 commits into from Feb 17, 2022
Merged

send_super fixes #220

merged 6 commits into from Feb 17, 2022

Conversation

samschott
Copy link
Member

This PR fixes #219.

It also aims to provide a uniform API between send_message and send_super. For send_super, argtypes is a keyword argument which defaults to an empty list. This can be convenient if there are no args provided to the call in the first place. I've therefore adapted this for send_message as well. Alternatively, one might make argtypes a mandatory argument for send_super. In any case, I would argue that a uniform API would be nice here.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Sam Schott added 4 commits February 15, 2022 21:14
This allows using versions of Python which are not preinstalled in the tools cache on GitHub-hosted runners.
Copy link
Member

@freakboy3742 freakboy3742 left a 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.

@samschott
Copy link
Member Author

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 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.

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.

Fair point, will do.

@freakboy3742
Copy link
Member

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.

@samschott
Copy link
Member Author

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.

@samschott samschott changed the title Send super fixes send_super fixes Feb 16, 2022
Copy link
Member

@freakboy3742 freakboy3742 left a 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!

@freakboy3742 freakboy3742 merged commit 9adc0d7 into beeware:master Feb 17, 2022
@samschott samschott deleted the send-super-fixes branch January 23, 2023 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce restype and argtypes for send_super
2 participants