Skip to content

fix: conversion to string in python bindings#19137

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
VadimLevin:dev/vlevin/safe-string-conversion
Dec 18, 2020
Merged

fix: conversion to string in python bindings#19137
opencv-pushbot merged 1 commit intoopencv:3.4from
VadimLevin:dev/vlevin/safe-string-conversion

Conversation

@VadimLevin
Copy link
Copy Markdown
Contributor

If provided PyObject can't be converted to string TypeError is reported instead of SytemError without any message.

Partially resolves #18835. but doesn't resolve problem with overload selection in general.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@VadimLevin
Copy link
Copy Markdown
Contributor Author

VadimLevin commented Dec 16, 2020

So limited API prevents us from getting type name directly and we can't access slot with allowed functions prior to Python 3.4...

 If provided `PyObject` can't be converted to string `TypeError` is
 reported instead of `SytemError` without any message.
@VadimLevin VadimLevin force-pushed the dev/vlevin/safe-string-conversion branch from ef3eb89 to 7b0d7d0 Compare December 16, 2020 12:12
@VadimLevin VadimLevin changed the title fix: converstion to string in python bindings fix: conversion to string in python bindings Dec 16, 2020
Copy link
Copy Markdown
Member

@alalek alalek 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 to me! Thank you 👍


def test_parse_to_string_convertible(self):
try_to_convert = partial(self._try_to_convert, cv.utils.dumpString)
for convertible in (None, '', 's', 'str', str(123), np.str('test1'), np.str_('test2')):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None

do we really need this?

Copy link
Copy Markdown
Contributor Author

@VadimLevin VadimLevin Dec 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All existed conversion functions are (even for trivial types such as int, float and etc.) very tolerant to None values in Python - treating them as "use default". In this test I wanted to explicitly notice this in test, because if this behaviour will be changed sometimes - it will be clear for API users - tests will document this.

Copy link
Copy Markdown
Member

@alalek alalek Dec 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"tested" usually means that feature is supported without future regressions.
"non-tested" - undocumented behavior, it may work for now, use it at own risk.

None handling is still not complete in Python binding implementation. See this comment from previous PR.
So this None case should go away after implementation got fixed.


"use default"

It should be for args with "defaults" only.

@opencv-pushbot opencv-pushbot merged commit b2ea15d into opencv:3.4 Dec 18, 2020
@alalek alalek mentioned this pull request Dec 18, 2020
@alalek alalek mentioned this pull request Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants