-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-94808: add tests covering PySequence_[InPlace_]Repeat
#99196
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
base: main
Are you sure you want to change the base?
Conversation
Merge conflicts are resolved, @serhiy-storchaka would you mind taking a look? :) |
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, except for few pep7 nitpicks.
On another hand, I'm not sure if this is a right approach to test abstract interfaces. Another example is PyNumber_*
API (#111996). Maybe we should use special, dedicated just for tests types, that have required magic methods, instead of using core types?
sequence_inplace_repeat(PyObject* self, PyObject *args) | ||
{ | ||
PyObject *sequence; | ||
Py_ssize_t count; |
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.
PEP 7: blank line after local variable declarations;)
sequence_repeat(PyObject* self, PyObject *args) | ||
{ | ||
PyObject *sequence; | ||
Py_ssize_t count; |
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.
And here.
Things I've skipped:
NULL
checks forPyLong
allocation andNULL
check forself
. Cannot be reproduced from pythonsq_repeat
andsq_inplace_repeat
, I expect them to be covered in*
and*=
operatorsCoverage view before:
