-
Notifications
You must be signed in to change notification settings - Fork 10
Proposal: Simplify container protocols #11
Comments
Thanks for thinking on this, @Lukas-Stuehrk. All of that repetition in the implementation is annoying and conspicuous, but it serves a specific purpose (at least I think it does). When I first approached the problem, a IIRC, what I eventually found was that It could very well be that I missed something obvious in my first implementation, or wasn't clever enough to find a better solution (as you saw, managing child nodes wasn't my primary use case, and thus wasn't the focus of my efforts). If you found a solution that simplified the implementation without degrading API usability, I'd be glad to have it. But I just wanted to offer fair warning that I tried the same thing before, and wasn't able to make it work. |
I did a quick prototype in the The first commit 7101648 removes all protocols and comes with a single container protocol. Type safety is still given, so it's not possible to add a block as a child of a container that only supports inline elements. If we still need to be able to have the All changes happen in However, I also understand that this implementation might use a certain level of "cleverness" which you might not want to have in a codebase, as it might be not obvious enough for people who are not familiar with the codebase. |
I stand corrected! 7101648 is exactly how this should be done. If you submit that in a PR, I'd be very happy to incorporate it for the next release. As for 1d89b50, I'm having trouble wrapping my head around why the |
The typealias declaration in 1d89b50 works because I kind of cheated: As for 7101648, I need to disclose one little problem: Please notice the ugly cast in line 60: 7101648#diff-6ec4af82405deaa118bdbc9885526abbR60 I'd prefer to modify the existing approach and use However, this cast should always work as it's impossible to add the wrong child. I will open a pull request for the simplification, but just wanted to be fully transparent with this problem and not sneak it in. |
I managed to find a more elegant solution for the problem. |
@Lukas-Stuehrk A fair amount of code has changed with #22 and more stands to change with #25, so this may no longer be a concern. If it is, please let me know and I'll reopen for further discussion. Thanks! |
There are two different protocols for container elements currently:
ContainerOfBlocks
ContainerOfInlineElements
Both protocols have extensions which implement exactly the same logic, except that the return and input types differ (
Block & Node
vs.Inline & Node
).On top of that, both
List
andList.Item
have the same extensions, again with different types. So there are basically four times the same implementations. Fixing bugs or adding functionality needs to be replicated four times.We could simplify this to use one single container protocol instead. The unified protocol would look like:
The different container elements would declare conformance with an implementation like:
The methods in an extension of the protocol would then look like this example for
insert(child:before:)
:What do you think? I can readily provide a pull request implementing the concept.
The text was updated successfully, but these errors were encountered: