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.

Sign up for GitHub

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

Jump to bottom

✨ zb,zm: Support Option<Header<'_>> parameter in property getters #1176

Merged
merged 3 commits into from
Dec 18, 2024
Merged

✨ zb,zm: Support Option<Header<'_>> parameter in property getters #1176

merged 3 commits into from
Dec 18, 2024

Conversation

rzvncj
Copy link
Contributor

@rzvncj rzvncj commented Dec 16, 2024

This allows for fine-grained connecting-UID checking, in order to be able to control who is able to access property values.

@zeenix zeenix changed the title ✨ zb: Add support for Option<Header<'_>> parameter in property getters ✨ zb: upport for Option<Header<'_>> parameter in property getters Dec 17, 2024
@zeenix zeenix changed the title ✨ zb: upport for Option<Header<'_>> parameter in property getters ✨ zb: Support Option<Header<'_>> parameter in property getters Dec 17, 2024
@zeenix zeenix changed the title ✨ zb: Support Option<Header<'_>> parameter in property getters ✨ zb,zm: Support Option<Header<'_>> parameter in property getters Dec 17, 2024
zeenix
zeenix reviewed Dec 17, 2024
View reviewed changes
Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

Really impressive work. I know it's quite difficult to get into the very complex and strange macro code and most contributors don't touch it.

Apart from the inline comments (mostly minor nitpicks), please add the new parameter to the interface docs and also to the book's service chapter.

zbus_macros/src/iface.rs Show resolved Hide resolved
zbus_macros/src/iface.rs Outdated Show resolved Hide resolved
zbus/tests/e2e.rs Outdated Show resolved Hide resolved
zbus/src/object_server/interface/mod.rs Show resolved Hide resolved
zbus_macros/src/iface.rs Outdated Show resolved Hide resolved
zeenix
zeenix requested changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

oops, used the wrong option. :)

@rzvncj rzvncj force-pushed the support-attributes-in-properties branch from 035601a to a43ca7e Compare December 17, 2024 12:38
@rzvncj
Copy link
Contributor Author

rzvncj commented Dec 17, 2024

Thanks for the prompt and very helpful review, and the kind words!

I've hopefully addressed your comments, other than updating the book and interface docs, and doing the "prequel" commit renaming the short parameter names - I'll be updating the PR with those changes as soon as possible too.

@rzvncj rzvncj force-pushed the support-attributes-in-properties branch from a43ca7e to 97fec7e Compare December 17, 2024 15:19
@rzvncj
Copy link
Contributor Author

rzvncj commented Dec 17, 2024

All done, I think, except for updating the book, because I'm not sure where this new information fits. Would you like me to update the MyGreeter example? Or a specific section? Or both?

@rzvncj rzvncj force-pushed the support-attributes-in-properties branch from 97fec7e to 3d9c1a0 Compare December 17, 2024 17:10
@zeenix
Copy link
Contributor

zeenix commented Dec 17, 2024

All done,

👍

I think, except for updating the book, because I'm not sure where this new information fits. Would you like me to update the MyGreeter example? Or a specific section? Or both?

I just had a look a the book again and seems we don't introduce all the special args there so never mind.

zeenix
zeenix requested changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

LGTM otherwise. Getting very close now. :)

zbus_macros/src/iface.rs Show resolved Hide resolved
zbus/src/object_server/interface/mod.rs Outdated Show resolved Hide resolved
zbus_macros/src/lib.rs Outdated Show resolved Hide resolved
zbus_macros/src/lib.rs Outdated Show resolved Hide resolved
@rzvncj
♻️ zm: Use more descriptive parameter names in call() and call_mut()
1ad5e51 
Signed-off-by: Razvan Cojocaru <rzvncj@gmail.com>
@rzvncj
✏️ zm: Minor typo fix
e2ec19b 
Signed-off-by: Razvan Cojocaru <rzvncj@gmail.com>
@rzvncj rzvncj force-pushed the support-attributes-in-properties branch from 3d9c1a0 to 082c2f3 Compare December 17, 2024 22:28
@rzvncj
Copy link
Contributor Author

rzvncj commented Dec 17, 2024

LGTM otherwise. Getting very close now. :)

Great, thanks! Submitted a new round of patches for review.

@rzvncj rzvncj force-pushed the support-attributes-in-properties branch 2 times, most recently from 3273241 to 088d643 Compare December 18, 2024 19:27
zeenix
zeenix reviewed Dec 18, 2024
View reviewed changes
zbus_macros/src/lib.rs Outdated Show resolved Hide resolved
zeenix
zeenix requested changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

LGTM otherwise. I guess we can merge once you fix this one tiny thing.

@zeenix zeenix enabled auto-merge December 18, 2024 19:56
auto-merge was automatically disabled December 18, 2024 19:58

Head branch was pushed to by a user without write access

@rzvncj rzvncj force-pushed the support-attributes-in-properties branch from 088d643 to 1d14cfa Compare December 18, 2024 19:58
@rzvncj
✨ zb,zm: Support Option<Header<'_>> parameter in property getters
1d14cfa 
This allows for fine-grained connecting-UID checking, in order to
be able to control who is able to access property values.

Signed-off-by: Razvan Cojocaru <rzvncj@gmail.com>
@zeenix zeenix enabled auto-merge December 18, 2024 20:00
@zeenix zeenix merged commit 60e64bd into dbus2:main Dec 18, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zeenix zeenix zeenix approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rzvncj @zeenix