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

feat: add unpack variants option #20

Merged
merged 8 commits into from
Sep 20, 2022
Merged

feat: add unpack variants option #20

merged 8 commits into from
Sep 20, 2022

Conversation

mdegat01
Copy link
Collaborator

@mdegat01 mdegat01 commented Sep 13, 2022

Add a flag to unpack variants in outputs. Supported this flag in all methods of the proxy interface (methods, properties and signals).

Support for this flag was not implemented in MessageBus.call. We could do this but it seemed like that one should remain raw. Also the wrappers in Proxy Interface won't work correctly if variants are unpacked by call so they have to handle it separately anyway.

@mdegat01 mdegat01 added the enhancement New feature or request label Sep 13, 2022
@mdegat01 mdegat01 requested a review from bdraco September 14, 2022 00:07
@mdegat01 mdegat01 force-pushed the remove-signature-flag branch from 5710372 to 749ebb9 Compare September 14, 2022 01:12
Copy link
Collaborator

@dlech dlech left a comment

Choose a reason for hiding this comment

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

This is something we are using in Bleak, so it could be nice to have it built-in to dbus-fast.

Although I could not understand what "remove signatures" meant until I read all of the code. In Bleak we called this "unpack_variants" since that is basically what it is doing. When I read "remove signatures", I thought this was doing some really low-level thing with the d-bus message itself.

src/dbus_fast/aio/proxy_object.py Outdated Show resolved Hide resolved
src/dbus_fast/proxy_object.py Outdated Show resolved Hide resolved
src/dbus_fast/proxy_object.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Base: 80.78% // Head: 80.88% // Increases project coverage by +0.09% 🎉

Coverage data is based on head (8bd423d) compared to base (ca2a3c1).
Patch coverage: 89.47% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #20      +/-   ##
==========================================
+ Coverage   80.78%   80.88%   +0.09%     
==========================================
  Files          24       24              
  Lines        2800     2835      +35     
  Branches      602      616      +14     
==========================================
+ Hits         2262     2293      +31     
  Misses        334      334              
- Partials      204      208       +4     
Impacted Files Coverage Δ
src/dbus_fast/__version__.py 0.00% <0.00%> (ø)
src/dbus_fast/glib/proxy_object.py 82.94% <76.47%> (-2.06%) ⬇️
src/dbus_fast/proxy_object.py 77.77% <94.73%> (+1.26%) ⬆️
src/dbus_fast/aio/proxy_object.py 96.42% <100.00%> (+0.42%) ⬆️
src/dbus_fast/signature.py 59.24% <100.00%> (+1.85%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mdegat01
Copy link
Collaborator Author

Unpack variants is a good name @dlech . As I mentioned above I was really just moving this code from supervisor into the library as it seemed really useful for everyone and kept the names we were using the same. But I'll rename it, that does make more sense.

@mdegat01
Copy link
Collaborator Author

So for the codecov check, its basically failing that because I also modified the glib part for completeness. And I also added tests for that but I see those aren't running. Should I just remove all changes to the glib and only change aio?

@bdraco
Copy link
Member

bdraco commented Sep 14, 2022

So for the codecov check, its basically failing that because I also modified the glib part for completeness. And I also added tests for that but I see those aren't running. Should I just remove all changes to the glib and only change aio?

I think we are missing a library being installed in the CI. We should probably fix that first. I'll see if I can fix it before I go to sleep but getting late here so not if I can before sleep.

@bdraco
Copy link
Member

bdraco commented Sep 14, 2022

GI tests should work now. CI should be a bit faster as well as caching will be turned on with this PR

#21

I'm too tired to be sure I got everything right with the jet lag so I didn't merge it, but feel free if it looks ok.

g'night

@mdegat01 mdegat01 force-pushed the remove-signature-flag branch from 1a7a4ad to 93ed41d Compare September 15, 2022 13:46
@mdegat01 mdegat01 changed the title feat: add remove signature option feat: add unpack variants option Sep 15, 2022
src/dbus_fast/constants.py Outdated Show resolved Hide resolved
Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

We need to address #20 (comment) before merging

@mdegat01 mdegat01 requested a review from bdraco September 15, 2022 17:52
Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mdegat01 mdegat01 force-pushed the remove-signature-flag branch from 17e4a01 to 0f3cfd2 Compare September 20, 2022 00:37
@mdegat01 mdegat01 force-pushed the remove-signature-flag branch from 0f3cfd2 to 8bd423d Compare September 20, 2022 00:38
@bdraco bdraco merged commit cfad28b into main Sep 20, 2022
@bdraco bdraco deleted the remove-signature-flag branch September 20, 2022 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants