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

Convert zvariant_utils macros to proc macros #820

Open
zeenix opened this issue Jun 2, 2024 · 14 comments
Open

Convert zvariant_utils macros to proc macros #820

zeenix opened this issue Jun 2, 2024 · 14 comments
Labels
enhancement New feature or request

Comments

@zeenix
Copy link
Contributor

zeenix commented Jun 2, 2024

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. 😠

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

@zeenix zeenix added the enhancement New feature or request label Jun 2, 2024
@turbocool3r
Copy link
Contributor

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

@zeenix
Copy link
Contributor Author

zeenix commented Jun 2, 2024

Sure

Awesome! 👍

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
Copy link
Contributor Author

zeenix commented Jun 29, 2024

@turbocool3r Hey, any luck?

@turbocool3r
Copy link
Contributor

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

@zeenix
Copy link
Contributor Author

zeenix commented Sep 11, 2024

Hopefully will get to it this weekend

Still no luck? :)

@zeenix
Copy link
Contributor Author

zeenix commented Sep 11, 2024

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. 🙏

@turbocool3r
Copy link
Contributor

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

@turbocool3r
Copy link
Contributor

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 turbocool3r@0ec3e83. 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
Copy link
Contributor Author

zeenix commented Sep 13, 2024

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? 🤔

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

Awesome! 👍

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. 😉

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
Copy link
Contributor

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
Copy link
Contributor Author

zeenix commented Sep 13, 2024

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? 🤔

@turbocool3r
Copy link
Contributor

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
Copy link
Contributor Author

zeenix commented Sep 15, 2024

@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
Copy link
Contributor

@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.

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

No branches or pull requests

3 participants