Skip to content

Comments

Refactoring as requested #1050

Closed
elmarco wants to merge 13 commits intoz-galaxy:mainfrom
elmarco:address
Closed

Refactoring as requested #1050
elmarco wants to merge 13 commits intoz-galaxy:mainfrom
elmarco:address

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?

@elmarco elmarco force-pushed the address branch 2 times, most recently from e1f7a2d to 33c4334 Compare October 7, 2024 12:52
@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 4 times, most recently from 8dda1d1 to f9b89be Compare October 8, 2024 19:28
@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 z-galaxy/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).

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

@zeenix
Copy link
Contributor

zeenix commented Oct 12, 2024

Also, you previously said your API patterns are consistent with similar API in std and it was actually one argument that had weight. Now you'll find that std API follows the pattern I'm insisting on here: parse once info a specific type and then access its attributes in a zero-cost way.

E.g SocketAddrV6. Not only are its getters zero-cost, they'reconst even.

I'm actually extremely surprised that you don't see a problem here. I was sure this is one issue you'll agree with me on.

@zeenix
Copy link
Contributor

zeenix commented Oct 13, 2024

Been thinking a lot more about this and realized two more things:

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 think you're probably correct here. However, this is because how the new API is designed. It's a workaround for a design flow of the new API.

For zbus itself, it's clearly unneeded.

Actually, this also affects zbus itself: you parse an Address given a string and then to connect to the address, you fetch the transport and guid, and for that you parse yet again. This is suboptimal and there is no good reason for it to be this way.

As requested by Zeeshan.
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 14, 2024

Nice. Thanks for the rework and addressing the concerns.

Does it already fix the re-parsing issue yet?

@elmarco
Copy link
Contributor Author

elmarco commented Oct 14, 2024

Actually, this also affects zbus itself: you parse an Address given a string and then to connect to the address, you fetch the transport and guid, and for that you parse yet again. This is suboptimal and there is no good reason for it to be this way.

Correct, because I didn't want to break API at the time of writing this series. If builder were to keep a reference of the original unparsed address, we wouldn't have this problem. The connection itself doesn't need to hold it after that.

See the new patches which now go directly from unparsed addresses to OwnedAddress'es.

Because the builder doesn't hold a reference to the address source, it
has to use an owned version. We can thus drop Address:to_owned(), and go
directly to OwnedAddress version.
This basic Guid type differs from zb::Guid, since it simply holds a
parsed bytes value.
@zeenix
Copy link
Contributor

zeenix commented Oct 14, 2024

Actually, this also affects zbus itself: you parse an Address given a string and then to connect to the address, you fetch the transport and guid, and for that you parse yet again. This is suboptimal and there is no good reason for it to be this way.

Correct, because I didn't want to break API at the time of writing this series. If builder were to keep a reference of the original unparsed address, we wouldn't have this problem.

How so? It still needs to parse the string first and then have Address reparses when the builder accesses the attributes of the address (each time for each attribute).

See the new patches which now go directly from unparsed addresses to OwnedAddress'es.

The ownership is not a problem here. You're solving the wrong problem. All types (wether owned or not) must only be parsed to, once.

I'm not sure how I can make it any clearer: The issue is reparsing after the Address has already been parsed from a string. That MUST NOT happen. If you've solved this issue or are planning to, please let me know. Otherwise, you're just wasting more of your and my time.

Without this issue resolved, I will not be going with the new API in 5.0 since I consider this a major flaw in the design of the API. I don't want to waste more time in trying to explain this to you.

@elmarco
Copy link
Contributor Author

elmarco commented Oct 14, 2024

How so? It still needs to parse the string first and then have Address reparses when the builder accesses the attributes of the address (each time for each attribute).

There is no reason to reparse if you correctly handle Address<'a> and Transport<'a> lifetimes.

See the new patches which now go directly from unparsed addresses to OwnedAddress'es.

The ownership is not a problem here. You're solving the wrong problem. All types (wether owned or not) must only be parsed to, once.

That's what OwnedAddress::new() does, it parses and store the result as owned values.

@zeenix
Copy link
Contributor

zeenix commented Oct 14, 2024

How so? It still needs to parse the string first and then have Address reparses when the builder accesses the attributes of the address (each time for each attribute).

There is no reason to reparse if you correctly handle Address<'a> and Transport<'a> lifetimes.

That makes no sense at all. This has nothing to do with lifetimes and owernership. Also, it has nothing to do with how the user uses it. If I do a let addr = Address::from_str(addr_str)?; then addr.transport() should be zero-cost.

See the new patches which now go directly from unparsed addresses to OwnedAddress'es.

The ownership is not a problem here. You're solving the wrong problem. All types (wether owned or not) must only be parsed to, once.

That's what OwnedAddress::new() does, it parses and store the result as owned values.

So you solved the issue for the new owned variants you just added but it does not solve the issue for the reference types. The owned variants should not have any difference to their reference types other than ownership of the internal state.


/// Universally-unique IDs for D-Bus addresses.
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct Guid([u8; 16]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WTH? That's not at all what we discussed and I never asked for a new type. I told you to use the existing Guid type. But forget it.

@zeenix
Copy link
Contributor

zeenix commented Oct 14, 2024

I'm not sure how I can make it any clearer: The issue is reparsing after the Address has already been parsed from a string. That MUST NOT happen. If you've solved this issue or are planning to, please let me know. Otherwise, you're just wasting more of your and my time.

Without this issue resolved, I will not be going with the new API in 5.0 since I consider this a major flaw in the design of the API. I don't want to waste more time in trying to explain this to you.

Since

  • I see the main issue here, as a major flaw in the new API
  • I have already spent a considerable amount of my time and energy trying to explain things to you
  • you refuse to even see the problem, let alone effectively solve it
  • now adding new unnecessary API (addres::Guid) that I didn't agree to

I'm now going ahead with the last resort I mentioned above: reverting the new address API for 5.0. We'll have plenty of time to get the API improved for 6.0.

Moreover, since we've already had a lot of fighting over this, I have to ask you to constraint yourself from commenting further on this topic. If my vision for zbus API is not something you can agree with, feel free to fork the project. That is your right and I will not protest. If you comment further on the topic, I might have to consider taking further drastic measures, such as blocking you (temporarily). I'd really hate to do that so please don't force me to do so.

@zeenix zeenix closed this Oct 14, 2024
@z-galaxy z-galaxy locked as too heated and limited conversation to collaborators Oct 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remaining issues with new address API

2 participants