Thanks everyone for the feedback. Basically everyone hated the idea, so I didn't do that. 🙂
Instead, I went with a fake-ADT approach, which allowed me to simplify a ton of code, remove a lot of duplicate code, and have only the slightest API break in an edge case I don't think anyone has actually used.
@gmazzap Ilija and I want to work on it and have a decent amount of design already done for it. No promise on timeline, but the rfc text for it and for pattern matching (which will be required) are already written. (The former needs some updates, of course.)
@Crell I’m torn on this kind of thing, too, and whenever I encounter this, I wish PHP had overloading, but I think that would cause a whole slew of new problems.
but its just a wrap around ItemAfter and ItemBefore for the days i feel like it. and i think my hasty condition check prefers after over before.
my editor is doing a fantastic job of handling the suggestions for named things. the AIO api fails a "is this a unit" smell test but i don't mind the AIO wrapper method existing for end-user end-app stuff.
@Crell for me, it isn’t enough that the proposed new version is better (if it is). To change a public API like that, even in a major version change, I would want the new version to be unambiguously a substantial improvement over the old version. I don’t know that this rises to that level.
@Crell I guess, but that only delays the inevitable updates for the user; the bar to force such a change, even eventually, should be at least “is unambiguously substantially better.”
@Crell when a method can take multiple parameters, and they’re all optuonal - that feels like a red flag to me. i like your current implementation better. a method name like addFooBefore shows intent over a universal addFoo
@Crell Why not take the opportunity and make the method names more descriptive? How about insertFoo() and appendFoo()? Or something more meaningful from the respective domain?
@robert They are descriptive in their domain at the moment. (It's event dispatcher registration. Add a listener add a listener with a sort priority, or add a listener before/after some other listener. Where most of the time I expect you don't care about order so don't use anything.)
@Crell If I had to choose I'd say this change is a bad idea. Having single method doesn't bring any value but confusion (call itself is readable tho).
That said, these methods require knowledge of internal structure, so they're either redundant (order of calls in local scope) or serve as a convenience that deals with temporal coupling.
I'd simply remove them, but I feel that "bad code should be hard to write" is not a popular philosophy in php world, so it's a matter of picking fights I guess.
@dantleech To make things more exciting, of course, there is a second optional argument other than $id ($type) that is also consistent across all 3 options. So... I suspect named args is going to always be a thing in practice. That's why I'm so tempted by just going all in on named args.
@Crell slightly easier (assuming we're talking about things like Rust Enums), but the safety (assuming no bugs from vendor) is the same to the user?
but also: what is going to cost more - user having to instantiate the Value Object once (but it will work forever) or user having to guess how to use it (e.g. somewhere before ID 12 but after 6! and then based on the given priority! oh wait...) and potentially debug runtime errors sooner or later.
No. Forgot the 4th parameter. Which would be the ID.
But that then requires too many args....
When they are all optional params, then your proposed "before: " or "after: "works fine but you will have to check whether both are provided to fail. Or define a default order. When both are provided, AFTER will win.
Using a special object that takes an enum and an id would probably be too much overkill though the cleanest solution...
@heiglandreas Yeah, that's over-engineered. I already have a one liner that confirms only one is non-null, for the attribute version. There are other optional args on all of them I left out of the example. All 3 have one required arg, a callable.
I predict most uses won't use any of the three args. They're all optional.
Most often, people will use an attribute instead that I have already combined into one, since those should always be using named args anyway. That makes the code a bit easier if they can be combined.
Usually best to let each function do a single task as is easier to maintain / test / etc. so keeping them separate is preferred.
However could make a new function similar to what you're suggesting that validates the input and then triggers the appropriate function based on those inputs.
End result would be using a single function throughout code that allows triggering any of the 3 functions.
@gmazzap@derickr What you're suggesting is essentially the "knock-off ADTs" discussed in another branch of the thread. I'm torn on if the API that results is convenient enough.
@Crell@derickr Indeed. If PHP enums would allow for constructor args you could use a enum for it ;) That would be the idiomatic approach in many other languages.
Add comment