Python wrapper - Explicit config enable_stream(...) to have unique arg count (API CHANGE)
#11861
Conversation
Nir-Az
changed the title
enable_stream(...) to have unique arg countenable_stream(...) to have unique arg count (API CHANGE)
|
I think one of our examples has one call with only one argument... have you checked? |
Missed that one, fixed |
| config = rs.config() | ||
| # Enable depth stream | ||
| config.enable_stream(rs.stream.depth) | ||
| config.enable_stream(rs.stream.depth, rs.format.z16, 30) |
There was a problem hiding this comment.
The old version was the one that was (stream_type,stream_index=-1), right? This is another version of enable_stream, right? Is it equivalent? Just want to make sure...
There was a problem hiding this comment.
Correct this use "stream_type"_a, "format"_a, "framerate"_a)
I will also run the example before merging it
There was a problem hiding this comment.
Verified
I wonder if we should add a python only 1 argument function.
stream type only.
Normally the user doesn't care about stream index.
Thoughts?
There was a problem hiding this comment.
It's an option, if we want better backwards compatibility. (and usage, I guess)
Should I wait before approving, then?
wrappers/python/pyrs_pipeline.cpp
Outdated
| "Before resolve() is called, no conflict check is done.", "stream_type"_a, "stream_index"_a, "width"_a, "height"_a, "format"_a, "framerate"_a) | ||
|
|
||
|
|
||
| .def("enable_stream", []( rs2::config* c, rs2_stream s ) -> void { return c->enable_stream( s, -1 ); }, "Stream type only. Other parameters are resolved internally.", "stream_type"_a ) |
There was a problem hiding this comment.
Why the explicit -1 instead of using the C++ enable_stream that takes only 1 arg? (i.e., it has a default second arg)
There was a problem hiding this comment.
Or it can be:
.def("enable_stream", (void (rs2::config::*)(rs2_stream)) &rs2::config::enable_stream, ...)
No? Not sure it'll work -- but if not, then just remove the explicit -1
There was a problem hiding this comment.
It can be yes, I wanted to show that I explicitly call this command arguments but without it it will also work the same.
Do you think it's preferred?
There was a problem hiding this comment.
The only thing I don't like is the explicit -1, because it presumes to know what the default is. Up to you how to do it.
2 participants
Python cannot distinguish between enums, all enums are treated as ints.
So we cannot have 2 similar functions with the same arg count that only change the enum type.
This PR enforce no default arguments in
enable_stream()functions (luckily we have only unique counts on those)