search

dbus2 / zbus

Rust D-Bus crate.
Other
355 stars 81 forks source link

Convert `zvariant_utils` macros to proc macros #820

Open zeenix opened 4 months ago

zeenix commented 4 months ago

At least def_attrs, which is quite complex for macro_rules. I was trying to modify it to allow multiple lists but it turned out to be a whole day of cleaning up the macro and still can't figure it out. :angry:

@turbocool3r Any chance you'll be able and willing to help with this?

turbocool3r commented 4 months ago

Sure, would you prefer if I add support for multiple lists or convert it to a proc macro?

zeenix commented 4 months ago

Sure

Awesome! :+1:

would you prefer if I add support for multiple lists or convert it to a proc macro?

We need both actually but please prioritize conversion to proc macro first please and then it'd be easier for me (or anyone else) to add multiple lists support, in case you run out of time.

The use case for multiple lists is #807, where we would want zvariant_derive macros to also support using zbus as the attribute name, in addition to zvariant.

BTW, don't forget to bump the major version of zvariant_utils and bump the required version in using crates. :)

zeenix commented 3 months ago

@turbocool3r Hey, any luck?

turbocool3r commented 3 months ago

@zeenix hey, didn't have a lot of free time lately. Hopefully will get to it this weekend

zeenix commented 3 weeks ago

Hopefully will get to it this weekend

Still no luck? :)

zeenix commented 3 weeks ago

The thing is that we're now preparing for 5.0 and it would be a perfect time for #807 but that's a bit difficult with these macros not supporting a list of attributes. I did spend a weekend trying to add this but I found the macro code to be very difficult. Since you're the author of these macros, I was hoping it would be easier task for you to handle this. As I wrote before, conversion to proc macros can wait.

However, if you don't see yourself having any time for this any time soon, that's perfectly understandable (it's your time after all) but I'd appreciate a more realistic answer so I can plan things accordingly. :pray:

turbocool3r commented 2 weeks ago

Hey, sorry for the long delay, totally forgot about this. Sure, I'll look into it this evening. 👍

turbocool3r commented 2 weeks ago

So I took a look and it really looks like adding support for lists is much easier than rewriting the def_attrs macro as a proc macro. That'll also need a new separate crate for utils derive macro (zvariant_derive_derive?).

I've written a patch to add support for multiple lists here https://github.com/turbocool3r/zbus/commit/0ec3e83c6bbbfb63470df9ee27c131626c2dcffc. It's quite small and simple, the macro is simplified even more now. Can you take a look? If you still prefer a proc macro, I'll try to make it.

zeenix commented 2 weeks ago

So I took a look and it really looks like adding support for lists is much easier than rewriting the def_attrs macro as a proc macro.

I thought as much. :)

That'll also need a new separate crate for utils derive macro (zvariant_derive_derive?).

I understood from @GuillaumeGomez that it wouldn't. Probably cause it's not a derive or attribute macro? :thinking:

I've written a patch to add support for multiple lists here turbocool3r@0ec3e83.

Awesome! :+1:

It's quite small and simple, the macro is simplified even more now. Can you take a look?

Sure but why not send a PR? :) Easier to review that way. BTW, a typo there: zbus, not zbuz. :wink:

If you still prefer a proc macro, I'll try to make it.

As I explained before, that'd be great but it doesn't need to block support for lists.

GuillaumeGomez commented 2 weeks ago

If it's proc-macro to be used in the same crate, you'll need to move them out to another crate (if that is the current situation).

zeenix commented 2 weeks ago

If it's proc-macro to be used in the same crate, you'll need to move them out to another crate (if that is the current situation).

hmm.. actually no. The macro will live in zvariant_utils and used by zvariant_derive and zbus_macros crates. @turbocool3r am I missing something? :thinking:

turbocool3r commented 2 weeks ago

If it's proc-macro to be used in the same crate, you'll need to move them out to another crate (if that is the current situation).

Thanks, I confused zvariant_utils and zvariant_derive a bit. That must be enough.

Sure but why not send a PR? :) Easier to review that way. BTW, a typo there: zbus, not zbuz. 😉

Sure, will do in a couple of minutes

zeenix commented 2 weeks ago

@turbocool3r Thanks so much for #986. In case you already had some WIP branch for this, please rebase it on latest main because it will likely have some conflicts with #988.

turbocool3r commented 2 weeks ago

@turbocool3r Thanks so much for #986.

Thanks for the review.

In case you already had some WIP branch for this, please rebase it on latest main because it will likely have some conflicts with #988.

I didn't yet, but I'll have it today or tomorrow. Didn't want to start it before #986 gets merged.