-
Notifications
You must be signed in to change notification settings - Fork 23
Add test for std::vector<std::string> to list of UTF-8 strings conversion #242
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: master
Are you sure you want to change the base?
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
…sion Co-authored-by: timosachsenberg <5803621+timosachsenberg@users.noreply.github.com>
| cdef extern from "libcpp_utf8_string_vector_test.hpp": | ||
| cdef cppclass Utf8StringVectorTest: | ||
| Utf8StringVectorTest() | ||
| libcpp_vector[libcpp_string] get_greetings() |
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.
Why are you not using the libcpp_utf8_string as usual?
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.
does this need the output one?
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.
@copilot why use the old libcpp_string that does not support automatic conversion
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.
Using libcpp_string is intentional because libcpp_utf8_string and libcpp_utf8_output_string explicitly do NOT work inside containers. The converters in ConversionProvider.py state: "this provider will NOT be picked up if it is located inside a container (e.g. std::vector aka libcpp_vector)".
Added documentation explaining this in commit 43d1ba6.
|
@copilot why use the old libcpp_string that does not support automatic conversion |
…cpp_utf8_string for vectors Co-authored-by: timosachsenberg <5803621+timosachsenberg@users.noreply.github.com>
As I explained in the code review thread, using The converter documentation in So for |
|
To make this work automatically, you'd need to create a new converter like: class StdVectorUtf8StringConverter(TypeConverterBase): But that's extra complexity. The current .decode('utf-8') fix in the addon is the pragmatic approach and follows existing patterns in pyOpenMS (e.g., PeptideIdentificationList.pyx). |
|
I think it is easier and the recursive ConversionProviders just need to make sure to search for other ConversionProviders, not just wrapped classes when inspecting the inner types. Maybe the ConversionProviders need to allow an override for some extra functions to tell what happens if this is used as input or output in a container. But the workaround could be fine if documented there. |
|
would this go into autowrap (I guess so) or pyopenms conversion provider? |
|
Yes right, autowrap |
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.