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

Remove ::address #982

Open
elmarco opened this issue Sep 10, 2024 · 10 comments
Open

Remove ::address #982

elmarco opened this issue Sep 10, 2024 · 10 comments
Labels
api break enhancement New feature or request zbus Issues/PRs related to zbus crate
Milestone

Comments

@elmarco
Copy link
Contributor

elmarco commented Sep 10, 2024

zbus itself needs to parse dbus addresses, but 99% of users will simply use the default buses, or connect with a string.

Hence, zbus shouldn't provide zbus::address.

Furthermore, parsing a dbus address is a very different job from handling the connection itself. There is not much to share.

zbus can offload that specialized responsibility to an external crate, improving code maintainability, reusability, and overall focus.

I propose to rebase #562 and cleanup some zbus-related code.

@zeenix
Copy link
Contributor

zeenix commented Sep 10, 2024

As I repeatedly explained on the Matrix channel, I don't want to talk about another crate here (that will happen later for sure). Please edit the description to solely focus on the API changes you want to see and their benefits.

@elmarco
Copy link
Contributor Author

elmarco commented Sep 10, 2024

As I repeatedly explained on the Matrix channel, I don't want to talk about another crate here (that will happen later for sure). Please edit the description to solely focus on the API changes you want to see and their benefits.

Drop ::address is quite explicit. zbus doesn't provide any dbus address parsing API. An external crate (really I can't name it?) can provide it.

This is a good opportunity for code split and reuse: other projects can simply want to validate a dbus address or extract precise details (for tools, configuration files etc).

dbus-addr has a lot of similarities with zbus::address. The APIs are similar: both provide address TryFrom<&str> & tranport() with a similar Transport enum and related variants.

Some notable API differences:

  • it doesn't provide builder APIs: you typically build from a string since it's much easier. It can be added trivially if necessary.
  • provides ;-separated list support, iterators etc for compliance with the spec
  • comprehensive transports parsing, independent of the host. Deciding which transport is available is not the job of the parser.
  • Transport is <'a>: efficient string handling with Cow<'a>. Only when decoding leads to different result are String allocated. This is a minor improvements, but still nice.
  • more precise error reporting. zbus can encapsulate as Error::InvalidAddress(String) to avoid exposing the details.

dbus-addr implementation was done with care wrt encoding, validation etc. (decoding is done per value, nonce-tcp is not conflated with tcp, etc.).

@elmarco elmarco changed the title Replace ::address code with dbus-addr Remove ::address Sep 10, 2024
@zeenix zeenix closed this as completed Sep 10, 2024
@zeenix
Copy link
Contributor

zeenix commented Sep 10, 2024

I gave you a clear path towards achieving your goals here but you want to focus on zbus using your crate, which I told you is not going to happen in the beginning.

@elmarco
Copy link
Contributor Author

elmarco commented Sep 10, 2024

So what is the plan to get there?

@zeenix
Copy link
Contributor

zeenix commented Sep 10, 2024

So what is the plan to get there?

Copy-pasting from #983:

Since you wouldn't budge, I had a look into dbus-addr. Looks mostly good so let's use it. However, I'll want:

  • co-ownership so I can maintain it when you disappear for months. :)
  • move into zbus space (we'll need to coordinate that).
  • renamed to zbus-address, so it's clear that it's part of the project and people don't hate us for having yet another dep. :)

Also some comments:

  • Can you please make use of doc-comment crate to keep the README and crate-level docs in sync, like we do in zbus crates?
  • Add an example code in the README, with a few different examples.

@zeenix
Copy link
Contributor

zeenix commented Sep 11, 2024

@elmarco so you happy with the above plan then?

@elmarco
Copy link
Contributor Author

elmarco commented Sep 11, 2024

@elmarco so you happy with the above plan then?

sounds good, I'll try to work on it by the end of the week. thanks!

@zeenix zeenix added enhancement New feature or request api break zbus Issues/PRs related to zbus crate labels Sep 11, 2024
@zeenix zeenix added this to the zbus-5.0 milestone Sep 11, 2024
@zeenix
Copy link
Contributor

zeenix commented Sep 17, 2024

Thinking a lot more about this, I realised I want to go in the complete opposite direction: #1006 . I know you'll likely not agree but if you did any maintenance work, you'd know the pain and extra work splitting of crates brings.

I also found out that dbus-addr did not find any users in its 8 months of existence, despite it being completely unrelated to zbus and not having "zbus" in its name even. Not that zbus_names does any better and its been around fort 3 years.

Anyway, I need to be shown clear evidence that this crate will most definitely be useful outside of zbus.

@elmarco
Copy link
Contributor Author

elmarco commented Sep 17, 2024

I believe the maintainance work can be helped with, and the development, debugging, biscecting etc is made easier when you split nicely. There are various tools to help management & release of large workspaces, for ex cargo-workspaces & cargo-smart-release. I have never used them, but I also work on other projects that have a large number of ws crates.

dbus-addr (or https://crates.io/crates/dbus-server-address-parser) don't have known users on crates.io, but there are regularly begin pulled.

One use case of usefulness for such crates is parsing & validating an address. For example, in configuration files. But the most obvious is certainly writing a different dbus client / server / proxy etc.

@zeenix
Copy link
Contributor

zeenix commented Sep 17, 2024

I believe the maintainance work can be helped with

Maybe but the question is if we want the pain at all in the first place. We should focus on justification for the split here IMO.

development, debugging, biscecting etc is made easier when you split nicely.

As the almost sole person doing all that, I don't really see that to be true. We've a mono-repo so the git history is common so I don't see how a split in the same repo would help with any of that.

dbus-addr (or https://crates.io/crates/dbus-server-address-parser) don't have known users on crates.io, but there are regularly begin pulled.

I don't really care much for benefits of proprietary users. I wouldn't want to spend a second just for benefiting them. See if you can find any users on github. Also, are you sure it's not mostly your own downloads? 😉

One use case of usefulness for such crates is parsing & validating an address. For example, in configuration files. But the most obvious is certainly writing a different dbus client / server / proxy etc.

I understand and agree with the use case (you don't need to convince me on that) but I fear it's mainly a theoretical one. How about we do it through features? We feature-gate everything so users can depend on/use zbus with only address feature (which disables everything in zbus except for address API).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api break enhancement New feature or request zbus Issues/PRs related to zbus crate
Projects
None yet
Development

No branches or pull requests

2 participants