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

Refactoring as requested #1050

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Refactoring as requested #1050

wants to merge 6 commits into from

Conversation

elmarco
Copy link
Contributor

@elmarco elmarco commented Oct 7, 2024

Introduce TransportTrait with from_address() and fmt_key_val(), as requested in #982.

Fixes #1064.

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.

Thanks. Some minor points but great otherwise. Also some of the feedback hasn't yet been addressed (e.g this and this) but I'm guessing it'll come in a following PR?

zbus/src/connection/connect/win32.rs Outdated Show resolved Hide resolved
zbus/src/address/transport/mod.rs Outdated Show resolved Hide resolved
zbus/src/address/transport/mod.rs Outdated Show resolved Hide resolved
zbus/src/address/transport/nonce_tcp.rs Show resolved Hide resolved
@zeenix
Copy link
Contributor

zeenix commented Oct 8, 2024

While working on the busd port, I realized some other issues with the new address API:

  • It uses Cow instead of zvariant::Str, which allows for avoiding allocations when built from static string. There is a very good reason it exists and why we're using it everywhere.
  • Address::guid returns a string, instead of our Guid type (which makes use of Str type), that's meant for exactly this.

Please address these along with other remaining concerns withing a few days time. I need this all sorted out ASAP so I can finish the release. Otherwise, I'll be forced to revert all your work.

@elmarco
Copy link
Contributor Author

elmarco commented Oct 8, 2024

  • Could you explain why Str is better than Cow with 'static str ? this is against the my design decision to make address parsing a standalone code.
  • Similarly Guid has some zbus-specifics which are not desirable for a generic parsing code. Wrapping the returned value should have no additional cost.

Fwiw, your tone has been concerning. Threatening to disregard the time and effort I have already invested, especially after you accepted it, is not an effective way to manage a project or its team members. This approach needs to change if I am to continue working with you. Thank you.

@zeenix
Copy link
Contributor

zeenix commented Oct 8, 2024

  • Could you explain why Str is better than Cow with 'static str ?

Because Cow clones/allocates on to_owned even for &'static str since it doesn't differentiates between &str and &'static str. Str does not.

this is against the my design decision to make address parsing a standalone code.

The code we ended up with in the end was not standalone at all, so those design decisions are very irrelevant now.

  • Similarly Guid has some zbus-specifics which are not desirable for a generic parsing code.

Again, you need to remove all aspects of this design since that's not what we went for.

Wrapping the returned value should have no additional cost.

Then why not do it for the user? The user should only see the specific type we already have.

Threatening to disregard the time and effort I have already invested

You know very well that you practically forced me to invest a lot of time reviewing changes too, which took a lot of my time as well. And it seems it continues to drain my limited time.

especially after you accepted it

Because you just won't give up about it, despite the fact that you completely failed to provide a single real world use case. Now I'll have to live with a over-complicated API in my project so the least you can do, is adjust to the project conventions in a timely manner.

I accepted the PR in the end, mainly a personal favour to you. If it was by someone else, I would have definitely rejected it.

is not an effective way to manage a project or its team members.

I'm sorry to inform you but you've not been a team member for a while now. If you were, you wouldn't be questioning Str's use here, which we use everywhere. I'm giving you a way to get back in by adjusting your new API to better fit with the rest of the project and its conventions.

Fwiw, your tone has been concerning.
This approach needs to change if I am to continue working with you.

I need to roll-out 5.0 soon. All public API changes need to happen before that but when I mention issues with your changes, you waste my time arguing about it, instead of trusting my judgements as the maintainer and just addressing the concerns.

I'd not use this tone, if you stop doing that and address the concerns and follow my lead as the maintainer.

@zeenix
Copy link
Contributor

zeenix commented Oct 8, 2024

I did some of the work here but I need to focus on a bunch of work work now. We need to replace Cow with Str in other places too then just Address struct itself.

@elmarco
Copy link
Contributor Author

elmarco commented Oct 8, 2024

Because Cow clones/allocates on to_owned even for &'static str since it doesn't differentiates between &str and &'static str. Str does not.

Why does it matter in the context of address parsing?

this is against the my design decision to make address parsing a standalone code.

The code we ended up with in the end was not standalone at all, so those design decisions are very irrelevant now.

It is still trivial to adjust the code to compile standalone and I would rather keep it that way. There is still a more valuable future where it can be used without the rest of zbus.

Again, you need to remove all aspects of this design since that's not what we went for.

Because you changed your mind, but the code does not rely on anything from the rest of zbus so far afaik.

Threatening to disregard the time and effort I have already invested

You know very well that you practically forced me to invest a lot of time reviewing changes too, which took a lot of my time as well. And it seems it continues to drain my limited time.

This is not an excuse.

especially after you accepted it

Because you just won't give up about it, despite the fact that you completely failed to provide a single real world use case. Now I'll have to live with a over-complicated API in my project so the least you can do, is adjust to the project conventions in a timely manner.

I accepted the PR in the end, mainly a personal favour to you. If it was by someone else, I would have definitely rejected it.

It would be a shame to refuse technically superior code. There are real use cases for parsing a dbus address despite you saying not willing to admit it.

is not an effective way to manage a project or its team members.

I'm sorry to inform you but you've not been a team member for a while now. If you were, you wouldn't be questioning Str's use here, which we use everywhere. I'm giving you a way to get back in by adjusting your new API to better fit with the rest of the project and its conventions.

Thanks, that's a bright way to respect contributors and users.

Fwiw, your tone has been concerning.
This approach needs to change if I am to continue working with you.

I need to roll-out 5.0 soon. All public API changes need to happen before that but when I mention issues with your changes, you waste my time arguing about it, instead of trusting my judgements as the maintainer and just addressing the concerns.

I'd not use this tone, if you stop doing that and address the concerns and follow my lead as the maintainer.

You don't communicate well enough your plans. How are we supposed to know 5.0 is going out soon? There are many things that could still be changed during API break period.

And how are the changes you are asking relevant for 5.0? It's only mostly internal refactoring afaict...

You keep me asking for things against my opinions, when you could just do it.

I am tired of being treated like a robot. It's unfair and unpleasant. I have spent a lot of time on zbus, and I plan to continue doing so, but not like this.

@zeenix
Copy link
Contributor

zeenix commented Oct 8, 2024

And how are the changes you are asking relevant for 5.0? It's only mostly internal refactoring afaict...

Sure not all of them are directly relevant but if I were to decide to (even temporarily) revert the new API, I need to decide that before 5.0. Your cooperation and timely actions, can ensure that I don't seriously consider that option.

Because Cow clones/allocates on to_owned even for &'static str since it doesn't differentiates between &str and &'static str. Str does not.

Why does it matter in the context of address parsing?

Not the parsing itself. You think of this as a purely "parsing" API. To me, it's a specific type representing an address and should be used for that instead of strings. I wanted to create a typed const for default address in busd, as an example and that was the reason I thought of this. I'm sure there can be other uses of this.

It's not different than other string-backed specific types we have in the code base.

I am tired of being treated like a robot. It's unfair and unpleasant. I have spent a lot of time on zbus, and I plan to continue doing so, but not like this.

If I treated you like a robot, I wouldn't have wasted hours of my time, explaining my concerns and reservations, often repeating again and again, would I? :)

@zeenix
Copy link
Contributor

zeenix commented Oct 8, 2024

It is still trivial to adjust the code to compile standalone and I would rather keep it that way. There is still a more valuable future where it can be used without the rest of zbus.

Valuable to who? I've been asking you time and again for any actual use case for this and you've failed to point out a single one. We keep going in circles with this purely academic goal of yours. Unless you have some new arguments that could possibly convince me, please stop bringing it up. You have wasted a huge amount of my time on this already. Otherwise, this will remain a non-goal for zbus and must not guide any API design.

@elmarco
Copy link
Contributor Author

elmarco commented Oct 8, 2024

It is still trivial to adjust the code to compile standalone and I would rather keep it that way. There is still a more valuable future where it can be used without the rest of zbus.

Valuable to who?

#989 (comment)

@elmarco
Copy link
Contributor Author

elmarco commented Oct 8, 2024

Sure not all of them are directly relevant but if I were to decide to (even temporarily) revert the new API, I need to decide that before 5.0. Your cooperation and timely actions, can ensure that I don't seriously consider that option.

Can you be more specific on your timeline. We shouldn't put ourself under that kind of stress.

Why does it matter in the context of address parsing?

Not the parsing itself. You think of this as a purely "parsing" API. To me, it's a specific type representing an address and should be used for that instead of strings. I wanted to create a typed const for default address in busd, as an example and that was the reason I thought of this. I'm sure there can be other uses of this.

The job of parsing should remain outside the rest of zbus usage. Why can't it be wrapped later on? This is already an almost 0-cost abstraction, it shouldn't change much to wrap it.

It's not different than other string-backed specific types we have in the code base.

But other string types are more directly visible on the bus as message data and must be serialized etc. This is arguably also done at later stage and could be independent from zbus in the first place. This is imho one of the reasons why zbus-names can't be adopted outside zbus easily.

@elmarco elmarco force-pushed the address branch 2 times, most recently from 4948d64 to 0db54ec Compare October 8, 2024 16:04
Use address::Error::MissingValue for missing address fields.

Use zbus::Error for errors that are not related to address
handling as such, but rather higher-level.
Allow users to catch specific error kinds.
@zeenix
Copy link
Contributor

zeenix commented Oct 8, 2024

Sure not all of them are directly relevant but if I were to decide to (even temporarily) revert the new API, I need to decide that before 5.0. Your cooperation and timely actions, can ensure that I don't seriously consider that option.

Can you be more specific on your timeline.

Not really. It's been in the making for a bit now and people are awaiting the new optimized zbus.

We shouldn't put ourself under that kind of stress.

Sure but you went silent for a good few days and given you history of disappearing completely for months and not finishing your PRs for years, I thought you might have disappeared again for another few months when you didn't even respond to simple questions on the matrix channel.

Why can't it be wrapped later on? This is already an almost 0-cost abstraction, it shouldn't change much to wrap it.

Because changing the return type of a method, is a breaking change.

It's not different than other string-backed specific types we have in the code base.

But other string types are more directly visible on the bus as message data and must be serialized etc.

I don't see that as a relevant fact. I presented a very real use case that I was implementing just yesterday.

This is arguably also done at later stage and could be independent from zbus in the first place.

The 🐮 s are visible in public API and there is no reason, this can't use zvariant::Str already. It also makes it inconsistent with the very clear pattern of preferring Str over Cow in the rest of the code.

This is imho one of the reasons why zbus-names can't be adopted outside zbus easily.

How so? Sure, you drag zvariant dep but we can always/easily split Str into its own crate, if there is sufficient demand. For now it's a niche use case and people who use zbus_names independently of zbus, don't mind this dep so much.

@elmarco
Copy link
Contributor Author

elmarco commented Oct 9, 2024

Can you be more specific on your timeline.

Not really. It's been in the making for a bit now and people are awaiting the new optimized zbus.

Without timeline, it's hardly possible to prioritize work. I won't take that pressure without an agreed timeline, with soft/hard freeze periods etc.

We shouldn't put ourself under that kind of stress.

Sure but you went silent for a good few days and given you history of disappearing completely for months and not finishing your PRs for years, I thought you might have disappeared again for another few months when you didn't even respond to simple questions on the matrix channel.

Incorrect, unfair and unrelated.

Why can't it be wrapped later on? This is already an almost 0-cost abstraction, it shouldn't change much to wrap it.

Because changing the return type of a method, is a breaking change.

I am not talking about changing the return type in a future release. I meant at a higher level, lower level types can be wrapped or converted as necessary.

But other string types are more directly visible on the bus as message data and must be serialized etc.

I don't see that as a relevant fact. I presented a very real use case that I was implementing just yesterday.

"I wanted to create a typed const for default address in busd".

I shared this goal, early on. I also said I wish to have compile-time check. It was not a priority though, you didn't show much interest in that, and even went against it since parsing code is not a separate anymore.

Without compile time check, you have to provide unsafe ctor API, which should be avoided as it may create future issues.

It's hardly a priority, "const fn" are slowly getting used.

Can't we simply have?

    pub unasafe const fn new_static(addr: &'static str) -> Self {
        Address {
            addr: Cow::Borrowed(addr),
        }
    }

This is arguably also done at later stage and could be independent from zbus in the first place.

The 🐮 s are visible in public API and there is no reason, this can't use zvariant::Str already. It also makes it inconsistent with the very clear pattern of preferring Str over Cow in the rest of the code.

In general, Cow is not visible in ::address API, but since decode_percents() returns it, it make sense to expose it.

What does Str brings? My understanding is that it provides the zv traits (unnecessary for parsing), and a peculiar handling of &str which leads a bit astray from the std ToOwned design: it doesn't return an "owned" string like all the ToOwned implementors (https://doc.rust-lang.org/std/borrow/trait.ToOwned.html#implementors). But since it doesn't implement ToOwned, I can accept it, I wish it was better documented to avoid the confusion.

What is the issue with converting Cow to Str as necessary?

@zeenix
Copy link
Contributor

zeenix commented Oct 9, 2024

Can you be more specific on your timeline.

Not really. It's been in the making for a bit now and people are awaiting the new optimized zbus.

Without timeline, it's hardly possible to prioritize work. I won't take that pressure without an agreed timeline, with soft/hard freeze periods etc.

We don't have regular cycles. It will take as long as it takes. I'm not asking you to ensure by any means to deliver quickly. I'm asking you to try. If I see that you're doing the work, I am willing to delay as long as needed. Also, you must answer questions on the matrix channel (especially if they affect my work/planning) and not ignore them.

We shouldn't put ourself under that kind of stress.

Sure but you went silent for a good few days and given you history of disappearing completely for months and not finishing your PRs for years, I thought you might have disappeared again for another few months when you didn't even respond to simple questions on the matrix channel.

Incorrect, unfair and unrelated.

If this was incorrect, why didn't you say so when I asserted this last many times? You know it's very much true and that's why you didn't correct me before.

And no, it's very much relevant. If you disappear for months, I wouldn't want to wait for you. That's very fair.

Because changing the return type of a method, is a breaking change.

I am not talking about changing the return type in a future release. I meant at a higher level, lower level types can be wrapped or converted as necessary.

Then I've no idea what you're talking about. The task about guid is simple: the getter should return Guid type. That's it.

I shared this goal, early on. I also said I wish to have compile-time check. It was not a priority though, you didn't show much interest in that, and even went against it since parsing code is not a separate anymore.

I don't recall what you're referring to but I was just in general not interested in complicating the address API. I'm still not happy about doing so now but at least we can make it better and more consistent with rest of the project code.

Without compile time check, you have to provide unsafe ctor API, which should be avoided as it may create future issues.

busd is likely the only one that would need this and an unsafe API is fine for the only use it will have. Tests can further ensure it is safe.

Can't we simply have?

    pub unasafe const fn new_static(addr: &'static str) -> Self {
        Address {
            addr: Cow::Borrowed(addr),
        }
    }

Because Str gives all the benefits of Cow, plus no allocation/copying on to_owned and it's consistent.

What does Str brings? My understanding is that it provides the zv traits (unnecessary for parsing)

No. Look, I already explained why we use it everywhere and if for no other reason, we use it here for consistency.

it doesn't return an "owned" string like all the ToOwned implementors (https://doc.rust-lang.org/std/borrow/trait.ToOwned.html#implementors)

We wanted it to implement ToOwned but that doesn't work. Try it and you'll find out. Hence why it has a to_owned method instead.

I wish it was better documented to avoid the confusion.

Sure, I an agree with that. Feel free to fix that.


Now I've explained the reasons for why Str should be used and I also started the work for you. That's as much time I'm willing to spend on this. Now please, modify the code to use Str.

@elmarco
Copy link
Contributor Author

elmarco commented Oct 9, 2024

Now I've explained the reasons for why Str should be used and I also started the work for you. That's as much time I'm willing to spend on this. Now please, modify the code to use Str.

You don't provide evidence why Str is necessary and why it can't be converted from Cow or &str when necessary.

I can see your point about consistency, but the downside are also big. It's an overly complex type when simple common type are good enough. It's against the general design to have parser code independent from the rest of zbus. It's unnecessarily complicated when you can just convert to Str as needed.

@elmarco
Copy link
Contributor Author

elmarco commented Oct 9, 2024

We wanted it to implement ToOwned but that doesn't work. Try it and you'll find out. Hence why it has a to_owned method instead.

It's not ToOwned, because it doesn't return an owned value, it's still a shared reference...

@elmarco
Copy link
Contributor Author

elmarco commented Oct 9, 2024

And no, it's very much relevant. If you disappear for months, I wouldn't want to wait for you. That's very fair.

Please stop arguing, that won't help. We have job, we have life. Can we simply agree on when we do things so we can prioritize?

@zeenix
Copy link
Contributor

zeenix commented Oct 9, 2024

Now I've explained the reasons for why Str should be used and I also started the work for you. That's as much time I'm willing to spend on this. Now please, modify the code to use Str.

You don't provide evidence why Str is necessary

Not sure why I need to provide you any evidence. I provided you with my reasons and you tried to argue against and failed to convince me. In such cases, it's my call as the maintainer. The most I can offer is try to explain my reasons. It's not my job to endlessly argue until you're convinced.

and why it can't be converted from Cow or &str when necessary.

If the underlying type is a Cow, I don't automatically get the advantage of Str with the type in quesiton (e.g Address). If I do address.to_owned() and address is created from a static string literal, it will allocate/clone.

I can see your point about consistency, but the downside are also big. It's an overly complex type when simple common type are good enough.

Seriously? It's barely 230 LOC. OTOH, your new address API is much more complex than we've in 4.x, with no real world use case to justify it even.

It's against the general design to have parser code independent from the rest of zbus.

🤦 It is not just a "parser code". You've repeated this like 100s of times now and I've repeated each time that I don't agree with this goal/ideal.

It's unnecessarily complicated when you can just convert to Str as needed.

It's not. It's basically just Cow with a tiny bit extra smartness.

We wanted it to implement ToOwned but that doesn't work. Try it and you'll find out. Hence why it has a to_owned method instead.

It's not ToOwned, because it doesn't return an owned value, it's still a shared reference...

String is a shared reference? That's news to me.

And no, it's very much relevant. If you disappear for months, I wouldn't want to wait for you. That's very fair.

Please stop arguing, that won't help. We have job, we have life. Can we simply agree on when we do things so we can prioritize?

Why don't you start? I basically said the same thing in a previous comment.


Tl'dr we're going for Str and Guid types being used in address. You can either go ahead and make it happen, or leave it to me. I'll either do that or if it turns out too much work than I'm willing to put on cleaning up your mess, I'll just revert the whole thing. Up to you!

@elmarco
Copy link
Contributor Author

elmarco commented Oct 9, 2024

If the underlying type is a Cow, I don't automatically get the advantage of Str with the type in quesiton (e.g Address). If I do address.to_owned() and address is created from a static string literal, it will allocate/clone.

That's the purpose of ToOwned. It's not to give you a shared reference. You can simply share the reference to Address. You can trivially convert from &str or Cow to Str afaict. I don't see a compelling reason to expose this zb/zv type directly for Address members.

🤦 It is not just a "parser code". You've repeated this like 100s of times now and I've repeated each time that I don't agree with this goal/ideal.

Because you changed your mind on a previously agreed plan. I didn't change mine.

It's not ToOwned, because it doesn't return an owned value, it's still a shared reference...

String is a shared reference? That's news to me.

What are you talking about... Str::to_owned() will return a shared reference to the underlying &'static str., which is not the design of ToOwned. Maybe it should have been called into_static() instead ? I am not sold on the right way to handle that.

I leave it to you to decide what to do. I fundamentally disagree with your design choices here, I don't see why I should do it.

@zeenix
Copy link
Contributor

zeenix commented Oct 9, 2024

🤦 It is not just a "parser code". You've repeated this like 100s of times now and I've repeated each time that I don't agree with this goal/ideal.

Because you changed your mind on a previously agreed plan. I didn't change mine.

Not at all. I never agreed that this purely parser-only API. It doesn't have to be either-or. I might have changed my mind on other things but not this.

@zeenix
Copy link
Contributor

zeenix commented Oct 9, 2024

String is a shared reference? That's news to me.

What are you talking about... Str::to_owned() will return a shared reference to the underlying &'static str.,

Ah, you were talking about Str. I thought you were talking about Cow since you're presenting an intended feature as some bad thing.

which is not the design of ToOwned.

  1. I disagree. A static ref is very much owned. Why do you think you use 'static lifetime to mark a type as owned?

  2. Maybe on a purely theoretical level, you're right but I don't care too much about theory when there are practical advantages to consider. People use to_owned to get a type with a static lifetime and that's still what is happening IMO.

  3. Also, practical speaking, there's no disadvantage. Str in most cases it's hidden under the more specific wrapper type and that's what we also want here.

@zeenix
Copy link
Contributor

zeenix commented Oct 10, 2024

Not looking for a fight but more a calm-head discussion, hence starting a new thread. :)

As I wrote in the PM earlier, I think I may have overlooked something very important in my review of #982, that would have helped me understand why you kept insisting that Address is purely a parser. It looks to me that while Address does parse the string it's constructed from, it's still not a parsed representation of an address but rather parses the underlying string each time, it is used. E.g each time I call the transport method, the string is parsed.

I think for zbus, this wouldn't really matter much but it would suck for busd and any external users of this API as they'd need to reuse the same address many times but parsing the string form only once. This realization also explains why you had to switch to String type for address in dbus2/busd#142

If the intention is so that users are expected to cache the return value of Address::transport, then that will also not be 0-cost operation since user will have to to_own the transport because you can't do self-referential types (easily) in Rust.

I'm curious to know your thoughts here. I hope you can see my concern here and also that you have some solution that could be acceptable to us both. I would also suggest that if we keep the Address type as it is, changing the name to Parser so it's clear (similarly for other types I guess).

@elmarco
Copy link
Contributor Author

elmarco commented Oct 11, 2024

I think for zbus, this wouldn't really matter much but it would suck for busd and any external users of this API as they'd need to reuse the same address many times but parsing the string form only once. This realization also explains why you had to switch to String type for address in dbus2/busd#142

As I mentionned, it's temporary, I haven't finished the conversion. But I expect busd to be mostly concerned with the string representation of the address(es) once server is listening anyway, and it shouldn't have to rebuild the string when queried.

If the intention is so that users are expected to cache the return value of Address::transport, then that will also not be 0-cost operation since user will have to to_own the transport because you can't do self-referential types (easily) in Rust.

True, but into_owned() should be trivial to add if necessary (I am not convinced it is).

@zeenix
Copy link
Contributor

zeenix commented Oct 11, 2024

I think for zbus, this wouldn't really matter much but it would suck for busd and any external users of this API as they'd need to reuse the same address many times but parsing the string form only once. This realization also explains why you had to switch to String type for address in dbus2/busd#142

As I mentionned, it's temporary, I haven't finished the conversion. But I expect busd to be mostly concerned with the string representation of the address(es) once server is listening anyway, and it shouldn't have to rebuild the string when queried.

I disagree here. Maybe that works for busd but it's not at all a good design to have a type that parses a string and then on subsequent uses, it parses yet again each time. It should be either completely parsing upfront or always dynamic (the former sounds the best). You can disagree with me about other aspects of Address API in 4.x, but you've to admit that it had this part done right: it only parsed once.

I had an idea: Why don't the types cache the indexes to the parts in the string that they're concerned with? That way, they can be reconstructed with zero cost after the first parsing?

If the intention is so that users are expected to cache the return value of Address::transport, then that will also not be 0-cost operation since user will have to to_own the transport because you can't do self-referential types (easily) in Rust.

True, but into_owned() should be trivial to add if necessary (I am not convinced it is).

? How does the user keep the transport around then?

@elmarco
Copy link
Contributor Author

elmarco commented Oct 11, 2024

I disagree here. Maybe that works for busd but it's not at all a good design to have a type that parses a string and then on subsequent uses, it parses yet again each time. It should be either completely parsing upfront or always dynamic (the former sounds the best). You can disagree with me about other aspects of Address API in 4.x, but you've to admit that it had this part done right: it only parsed once.

Why would it have to parse several times? If you need to keep around the parsed / break down version (what' s the case?), we could have into_owned(). After all, Cow just allows this choice: either borrowed or owned.

I had an idea: Why don't the types cache the indexes to the parts in the string that they're concerned with? That way, they can be reconstructed with zero cost after the first parsing?

That's a hack, but sometime you don't have the choice. However, it does not keep a reference to the original string (the whole point), which is awkward and error-prone.

True, but into_owned() should be trivial to add if necessary (I am not convinced it is).

? How does the user keep the transport around then?

What do you mean? like any owned value.

@zeenix
Copy link
Contributor

zeenix commented Oct 11, 2024

I disagree here. Maybe that works for busd but it's not at all a good design to have a type that parses a string and then on subsequent uses, it parses yet again each time. It should be either completely parsing upfront or always dynamic (the former sounds the best). You can disagree with me about other aspects of Address API in 4.x, but you've to admit that it had this part done right: it only parsed once.

Why would it have to parse several times?

That's what I'm saying. They shouldn't need to. They parse it once and then keep and use the parsed form from then onwards. I hope you don't mean to say that they should only access address details once. That would be a needless restriction on the user.

If you need to keep around the parsed / break down version (what' s the case?), we could have into_owned(). After all, Cow just allows this choice: either borrowed or owned.

That still doesn't solve the issue of having to parse each time you access Address components.

I had an idea: Why don't the types cache the indexes to the parts in the string that they're concerned with? That way, they can be reconstructed with zero cost after the first parsing?

That's a hack,

I don't think so. The Address knows and keeps the string and in the first parsing, it figures out where the transport,guide etc is in the string. This string then remains immutable so any information we keep around about it, is guaranteed to stay the valid.

but sometime you don't have the choice.

If you have a better idea, I'm all ears. I don't want to be pushy here but this is a problem we need resolved. Otherwise, it's a regression from 4.x API.

However, it does not keep a reference to the original string (the whole point), which is awkward and error-prone.

I wasn't suggesting that. It's not even possible without awkward unsafe code and we'll need to get Pin involved and that could get complicated.

True, but into_owned() should be trivial to add if necessary (I am not convinced it is).

? How does the user keep the transport around then?

What do you mean? like any owned value.

I meant that your point about into_owned() do not address the main concern here.

@elmarco
Copy link
Contributor Author

elmarco commented Oct 11, 2024

That's what I'm saying. They shouldn't need to. They parse it once and then keep and use the parsed form from then onwards. I hope you don't mean to say that they should only access address details once. That would be a needless restriction on the user.

Yes, it's a well thought restriction. Can you give me a reason why the broken down version needs to be kept around? For zbus itself, it's clearly unneeded. For busd, I have claimed that what you need is the actual listenable address string, not the broken down/parsed address.

@zeenix
Copy link
Contributor

zeenix commented Oct 11, 2024

That's what I'm saying. They shouldn't need to. They parse it once and then keep and use the parsed form from then onwards. I hope you don't mean to say that they should only access address details once. That would be a needless restriction on the user.

Yes, it's a well thought restriction.

I don't see any thoughts and seems very unnecessary and arbitrary.

Can you give me a reason why the broken down version needs to be kept around?

I didn't say it needs to be kept around, necessarily. The suggestion I made as a solution, doesn't even involve that. To explain once again: parsing needs to happen once and then user should have a parsed type that they can keep around and use to access attributes of an address w/o any re-parsing (preferably completely 0-cost).

How we achieve that, is up to us. Unless you've any better solutions, I'd suggest we go for the simple one I presented already, which as I explained is infallible due to the underlying string being immutable.

For zbus itself, it's clearly unneeded.

Yes, I said that already. It's a concern for the zbus users though who create connection for a given address. busd is just one example that's well-known to us and completely in our control.

For busd, I have claimed that what you need is the actual listenable address string, not the broken down/parsed address.

Yes but that's not something I agree with. I think we should use strong-typing, wherever possible. From the busd pov, going to using a raw string to represent an Address is a regression IMO.


It's clear to me that we don't see eye-to-eye on this either. I won't be able to convince you that this is a valid concern and you won't be able to convince me that it isn't. As I wrote before, I don't want to start another endless fight again. So I'd request you focus on addressing my concern (either by going for my proposed solution or proposing your own).

@elmarco
Copy link
Contributor Author

elmarco commented Oct 11, 2024

That's what I'm saying. They shouldn't need to. They parse it once and then keep and use the parsed form from then onwards. I hope you don't mean to say that they should only access address details once. That would be a needless restriction on the user.

Yes, it's a well thought restriction.

I don't see any thoughts and seems very unnecessary and arbitrary.

Once again, we go in circles. Believe it or not I thought about the various use cases.

I didn't say it needs to be kept around, necessarily. The suggestion I made as a solution, doesn't even involve that. To explain once again: parsing needs to happen once and then user should have a parsed type that they can keep around and use to access attributes of an address w/o any re-parsing (preferably completely 0-cost).

And my question remain, why do you need to keep the parsed down version around?

It's clear to me that we don't see eye-to-eye on this either. I won't be able to convince you that this is a valid concern and you won't be able to convince me that it isn't. As I wrote before, I don't want to start another endless fight again. So I'd request you focus on addressing my concern (either by going for my proposed solution or proposing your own).

If I didn't have thought about the problem already, I would probably do as requested and be done. But I did, and I still claim that there are better ways to solve those issues.

@elmarco
Copy link
Contributor Author

elmarco commented Oct 11, 2024

From the busd pov, going to using a raw string to represent an Address is a regression IMO.

I agree with that, that's why Address can only hold a valid address.

@zeenix
Copy link
Contributor

zeenix commented Oct 11, 2024

That's what I'm saying. They shouldn't need to. They parse it once and then keep and use the parsed form from then onwards. I hope you don't mean to say that they should only access address details once. That would be a needless restriction on the user.

Yes, it's a well thought restriction.

I don't see any thoughts and seems very unnecessary and arbitrary.

Once again, we go in circles. Believe it or not I thought about the various use cases.

That's great but we still have with this issue.

I didn't say it needs to be kept around, necessarily. The suggestion I made as a solution, doesn't even involve that. To explain once again: parsing needs to happen once and then user should have a parsed type that they can keep around and use to access attributes of an address w/o any re-parsing (preferably completely 0-cost).

And my question remain, why do you need to keep the parsed down version around?

So they can access address attributes directly or indirectly (e.g when constructing a connection from the address). Some users (think systemd codebase) likes to creates lots of connections.

It's also in general a bad design. The string was already parsed. Why would it need to be parsed over and over again after that? It makes no sense to me. I seriously doubt you can change my mind about this.

I really think I've expressed why this is needed. Can we stop arguing about the need for this and focus on solving the problem?

If I didn't have thought about the problem already, I would probably do as requested and be done. But I did, and I still claim that there are better ways to solve those issues.

I'm not sure what you mean here. If you're going for my suggestion, please let me know and we can stop arguing. If not, please suggest a better way that would address my concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remaining issues with new address API
2 participants