-
-
Notifications
You must be signed in to change notification settings - Fork 83
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
base: main
Are you sure you want to change the base?
Refactoring as requested #1050
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As requested by Zeeshan.
As requested by Zeeshan.
While working on the busd port, I realized some other issues with the new address API:
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. |
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. |
Because
The code we ended up with in the end was not standalone at all, so those design decisions are very irrelevant now.
Again, you need to remove all aspects of this design since that's not what we went for.
Then why not do it for the user? The user should only see the specific type we already have.
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.
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.
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
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. |
As requested by Zeeshan.
I did some of the work here but I need to focus on a bunch of work work now. We need to replace |
Why does it matter in the context of address parsing?
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.
Because you changed your mind, but the code does not rely on anything from the rest of zbus so far afaik.
This is not an excuse.
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.
Thanks, that's a bright way to respect contributors and users.
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 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. |
As requested by Zeeshan.
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.
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.
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? :) |
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. |
|
Can you be more specific on your timeline. We shouldn't put ourself under that kind of stress.
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.
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. |
4948d64
to
0db54ec
Compare
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.
Not really. It's been in the making for a bit now and people are awaiting the new optimized zbus.
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.
Because changing the return type of a method, is a breaking change.
I don't see that as a relevant fact. I presented a very real use case that I was implementing just yesterday.
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.
How so? Sure, you drag zvariant dep but we can always/easily split |
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.
Incorrect, unfair and unrelated.
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.
"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?
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? |
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.
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.
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 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.
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.
Because Str gives all the benefits of Cow, plus no allocation/copying on to_owned and it's consistent.
No. Look, I already explained why we use it everywhere and if for no other reason, we use it here for consistency.
We wanted it to implement
Sure, I an agree with that. Feel free to fix that. Now I've explained the reasons for why |
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. |
It's not ToOwned, because it doesn't return an owned value, it's still a shared reference... |
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? |
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.
If the underlying type is a Cow, I don't automatically get the advantage of
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 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 not. It's basically just Cow with a tiny bit extra smartness.
String is a shared reference? That's news to me.
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! |
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.
Because you changed your mind on a previously agreed plan. I didn't change mine.
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. |
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. |
Ah, you were talking about Str. I thought you were talking about Cow since you're presenting an intended feature as some bad thing.
|
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 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 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 |
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.
True, but into_owned() should be trivial to add if necessary (I am not convinced it is). |
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 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?
? How does the user keep the transport around then? |
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.
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.
What do you mean? like any owned value. |
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.
That still doesn't solve the issue of having to parse each time you access
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.
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.
I wasn't suggesting that. It's not even possible without awkward unsafe code and we'll need to get
I meant that your point about into_owned() do not address the main concern here. |
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. |
I don't see any thoughts and seems very unnecessary and arbitrary.
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.
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.
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). |
Once again, we go in circles. Believe it or not I thought about the various use cases.
And my question remain, why do you need to keep the parsed down version around?
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 agree with that, that's why Address can only hold a valid address. |
That's great but we still have with this issue.
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?
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. |
Introduce TransportTrait with from_address() and fmt_key_val(), as requested in #982.
Fixes #1064.