remove dbus prefix

[TTWNO]

This removes the dbus_ prefix before macros and macro attributes.

Fixes #477

Fixes #478

[TTWNO]

This should fix #477 and #478

[TTWNO]

Oh, is it possible I misread it initially?

You want the attributes to be prefixed with zbus_ and the macros to be un-prefixed, yes?

[zeenix]

Oh, is it possible I misread it initially?

You want the attributes to be prefixed with zbus_

No, just zbus, similar to serde's attributes (I think zvariant too).

and the macros to be un-prefixed, yes?

Correct.

[zeenix]

This should fix #477 and #478

Nice! Please add Fixes.. line to the description of the PR so the issues get automatically closed when it's merged.

Thanks for doing this work. I'll try to review soon..

[zeenix]

Also kindly squash fixups to the relevant commits and force push, as per the contribution guide.

[TTWNO]

I will sqash in the AM (MDT)

[zeenix]

I will sqash in the AM (MDT)

I know life can get in the way so no pressure but just wanted to ping you, in case your forgot. :)

[zeenix]

btw, there's also a formatting issue breaking the CI.

[TTWNO]

Ddn't forget, jus time getting away from me.

[TTWNO]

I notice you mention in some of the TODOs that you want it to look like serde. What exactly did you mean by that?

[TTWNO]

Ah, I missed one thing, let me fix that up.

[zeenix]

I notice you mention in some of the TODOs that you want it to look like serde. What exactly did you mean by that?

Not sure which TODOs you are referring to but in this case, you'd notice that serde uses serde as the attribute name in its macros and we should be doing the same.

[TTWNO]

Ok, so I know you want me to keep the dbus_* crate names for the attributes, so that's the next thing I'm working on, but all attributes can be written as #[zbus(...)] now.

[TTWNO]

Do you want both so it's not a breaking change? So attributes could be either zbus(...) or dbus_*(...)

[zeenix]

Do you want both so it's not a breaking change? So attributes could be either zbus(...) or dbus_*(...)

Yes. I want these changes to be non-breaking. We should also emit a warning for use of existing names that they're deprecated. See commit 5db2d13 where we did something similar using eprintln.

[TTWNO]

Alright, all done for dbus_poxy/proxy and dbus_interface/interface.

Is using eprintln! preferable to using #[deprecated = "..."]? Or does it not matter?

[zeenix]

Alright, all done for dbus_poxy/proxy and dbus_interface/interface.

Nice! Thanks.

Is using eprintln! preferable to using #[deprecated = "..."]? Or does it not matter?

Actually deprecated is even better, as long as it actually works. I think the reason we used eprintln! in 5db2d13 was that it was about attributes inside the macro.

[zeenix]

Will take a deeper look once the CI succeeds but in the meantime please fix the commit log of the first commit. It would also be good to have Fix #ISSUE in each commit (as per the commit message guidelines linked in the contributors guide).

[TTWNO]

Actually deprecated is even better, as long as it actually works. I think the reason we used eprintln! in 5db2d13 was that it was about attributes inside the macro.

Yes, that's right. I fear that may be the only solution for dbus_error, which is still not done yet due to it being slightly more complex. I'll send a message when I think it's ready for CI, no need to keep running it for now :)

[TTWNO]

Ok, it looks like a clippy warning might be coming from zvariant, but it looks like some exports that are meant to be there.

warning: unused import: `from_value::*`
  --> zvariant/src/lib.rs:85:9
   |
85 | pub use from_value::*;
   |         ^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused import: `into_value::*`
  --> zvariant/src/lib.rs:88:9
   |
88 | pub use into_value::*;
   |         ^^^^^^^^^^^^^

[TTWNO]

Of course, it's easy to just remove the exports... but it looks purposeful.

[zeenix]

Actually deprecated is even better, as long as it actually works. I think the reason we used eprintln! in 5db2d13 was that it was about attributes inside the macro.

Yes, that's right. I fear that may be the only solution for dbus_error,

Oh, nobody said anything about DBusError. I'm not sure what can be done about that because zbus::Error is already taken and their both different things. So I decided to leave it be.

Ok, it looks like a clippy warning might be coming from zvariant

I think I fixed these recently. Can you try rebasing on current main and see if it helps?

[zeenix]

Oh, I see you squashed both commits now. :( This should be at least 2 commits, with their own justification and Fixes #ISSUE in their commit log. Which is why there were two issues created.

[zeenix]

Ok, it looks like a clippy warning might be coming from zvariant

I think I fixed these recently. Can you try rebasing on current main and see if it helps?

Seems you did that already? The CI passes now. 🥳 Now, if only you hadn't squashed the 2 commits and instead addressed my comments, I could have merged already. Could you please split the commits and redo the logs for each?

While at it, you might also want to add emoji prefixes as the contribution guide. :)

[zeenix]

@TTWNO Not only dbus_proxy doesn't work anymore, if you use it, you get warnings about the attribute as well even if you only use zbus attribute everywhere. With this change:

diff --git a/zbus/tests/e2e.rs b/zbus/tests/e2e.rs
index 730d576b..ddb81ff5 100644
--- a/zbus/tests/e2e.rs
+++ b/zbus/tests/e2e.rs
@@ -22,10 +22,9 @@ use zbus::{
 use zvariant::{DeserializeDict, Optional, OwnedValue, SerializeDict, Str, Type, Value};
 
 use zbus::{
-    connection, interface,
+    connection, dbus_proxy, interface,
     message::Header,
     object_server::{InterfaceRef, SignalContext},
-    proxy,
     proxy::CacheProperties,
     Connection, ObjectServer,
 };
@@ -52,7 +51,7 @@ pub struct RefType<'a> {
     field1: Str<'a>,
 }
 
-#[proxy(assume_defaults = true, gen_blocking = true)]
+#[dbus_proxy(assume_defaults = true, gen_blocking = true)]
 trait MyIface {
     fn ping(&self) -> zbus::Result<u32>;
 

I get this:

The `#[dbus_proxy(...)]` attribute of proxy has been deprecated in favor of `#[zbus(...)]`.
The `#[dbus_proxy(...)]` attribute of proxy has been deprecated in favor of `#[zbus(...)]`.
// Repeats 149 times!
error: use of deprecated macro `dbus_proxy`: Use #[proxy(...)] instead.
  --> zbus/tests/e2e.rs:54:3
   |
54 | #[dbus_proxy(assume_defaults = true, gen_blocking = true)]
   |   ^^^^^^^^^^
   |
   = note: `-D deprecated` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(deprecated)]`

error: implementation of `Deserialize` is not general enough
  --> zbus/tests/e2e.rs:54:1
   |
54 | #[dbus_proxy(assume_defaults = true, gen_blocking = true)]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ implementation of `Deserialize` is not general enough
   |
   = note: `RefType<'_>` must implement `Deserialize<'0>`, for any lifetime `'0`...
   = note: ...but `RefType<'_>` actually implements `Deserialize<'1>`, for some specific lifetime `'1`

error[E0599]: no method named `cached_count` found for struct `MyIfaceProxy` in the current scope
   --> zbus/tests/e2e.rs:653:22
    |
54  | #[dbus_proxy(assume_defaults = true, gen_blocking = true)]
    | ---------------------------------------------------------- method `cached_count` not found for this struct
...
653 |     assert_eq!(proxy.cached_count()?, None);
    |                      ^^^^^^^^^^^^ help: there is a method with a similar name: `set_count`

error[E0599]: no method named `receive_count_changed` found for struct `MyIfaceProxy` in the current scope
   --> zbus/tests/e2e.rs:772:18
    |
54  | #[dbus_proxy(assume_defaults = true, gen_blocking = true)]
    | ---------------------------------------------------------- method `receive_count_changed` not found for this struct
...
772 |     my_obj_proxy.receive_count_changed().await;
    |                  ^^^^^^^^^^^^^^^^^^^^^ method not found in `MyIfaceProxy<'_>`

error[E0599]: no method named `cached_count` found for struct `MyIfaceProxy` in the current scope
   --> zbus/tests/e2e.rs:775:18
    |
54  | #[dbus_proxy(assume_defaults = true, gen_blocking = true)]
    | ---------------------------------------------------------- method `cached_count` not found for this struct
...
775 |     my_obj_proxy.cached_count()?;
    |                  ^^^^^^^^^^^^ help: there is a method with a similar name: `set_count`

error[E0599]: no method named `cached_count` found for struct `MyIfaceProxy` in the current scope
   --> zbus/tests/e2e.rs:777:29
    |
54  | #[dbus_proxy(assume_defaults = true, gen_blocking = true)]
    | ---------------------------------------------------------- method `cached_count` not found for this struct
...
777 |     assert_eq!(my_obj_proxy.cached_count()?, Some(0));
    |                             ^^^^^^^^^^^^ help: there is a method with a similar name: `set_count`

[zeenix]

BTW, I moved both issue back on the agenda for 4.0, which I really want to rollout early-mid next week (I took the week off from work for that) as I realized it won't be nice to deprecate the most commonly used API in a stable release and we're close here, despite the issues.

I think something that would help is:

• supporting existing attributes only with the existing (deprecated) macros.
• mention the new attribute in the macro deprecation warning.
• because of the above 2, no need for deprecation warning for attributes.

@TTWNO Can you handle that before next week? If not, let me know and I'll see if I can do the rest. I'll not touch your branch anymore until you let me know.

[zeenix]

@TTWNO oh and feel free to add commits on top. I'll squash them into the appropriate commit for you. Please just try your best to keep them as small as possible, so it's easy to squash.

[TTWNO]

Can you handle that before next week?

Yes. It may come in at the last minute. But this will be done before Sunday evening (MST).

Super happy to see 4.0 coming together :)

[zeenix]

Can you handle that before next week?

Yes. It may come in at the last minute. But this will be done before Sunday evening (MST).

Awesome. I hope you don't hate me for this but thinking more about the new plan, I think it's now just one commit since we're no longer renaming the attribute for existing macros. So don't worry about splitting commits anymore. I wish I had come up with this plan before but I only got the idea after I tried the changes.

[TTWNO]

On this right now :)

[TTWNO]

• supporting existing attributes only with the existing deprecated macros.
• mention the new attribute in the macro deprecation warning.
• because of the above 2, no need for deprecation warning for attributes.

Done for interface and proxy macros. That error macro has me stumped.

[zeenix]

Done for interface and proxy macros.

Great, thanks. The CI is still failing though. Also, I'm sure you can squash commits into 1 (you did that recently). 😉

That error macro has me stumped.

You mean what I commented or how to handle it? 🤔

[TTWNO]

You mean what I commented or how to handle it? 🤔

How to handle it.

[zeenix]

You mean what I commented or how to handle it? 🤔

How to handle it.

If you could elaborate a little, I might be able to help.

[zeenix]

You mean what I commented or how to handle it? 🤔

How to handle it.

If you could elaborate a little, I might be able to help.

I think I know what you mean. Given that we're not renaming DBusError and that it's not used as often as the other macros, I think we could just break the API here and rename the attributes w/o providing backwards compat.

[zeenix]

I think I know what you mean. Given that we're not renaming DBusError and that it's not used as often as the other macros, I think we could just break the API here and rename the attributes w/o providing backwards compat.

And this one can and should go in a separate commit.

[TTWNO]

Got it. I'll get that dome this morning.

The rename for the derive should be to Error, yes?

[zeenix]

Got it. I'll get that dome this morning.

👍

The rename for the derive should be to Error, yes?

Just to be sure, I don't end up causing any misunderstanding:

• the derive macro itself does not change name as it's identical to the trait it implements.
• the attribute of the macro get renamed from dbus_error to zbus.
• the zbus_error value of zbus attribute should just become error, i-e #[zbus(error)].

[TTWNO]

Thanks for being patient. Answering you in class to make sure this gets into 4.0 :)

[TTWNO]

This is now a breaking change, due to dbus_error(zbus) has been replaced with zbus(error); no deprecations.

[zeenix]

Thanks for being patient.

Thanks for your patience too. I'm guessing I should handle the git surgery? :)

Answering you in class to make sure this gets into 4.0 :)

👍

[zeenix]

@TTWNO btw, I would invite you to our matrix channel as there are communications that are best handled on chat, rather than here.

[zeenix]

Now that I've modified your changes and split into commits, I approve it. :)

[TTWNO]

This looks better! Thank you for being patient with me and helping getting it over the finish line.