bkiers
All Filter and Tag registration via interface
I'd like to use this library in a JVM language called CFML. I can easily wrap up my custom classes to present as any interface on the classpath, but there is not currently a way in CFML for my classes to extend a given Java base class. This makes it pretty much impossible for me to extend this lib with Tags or Filters written in CFML.
I think this could be done in a backwards compat manner if an ITag and IFilter interface is defined. Implement those interfaces in your base abstract Tag and Filter classes and then change registerTag() and registerFilter() and all other internal variables to be typed to the interface instead. This will allow both sub-classing and interface-based approaches to extension.
@bdw429s Adding ITag and IFilter abstraction is not hard. But It's hard to do something without a clear understanding of all the requirements(a lot of questions like "should be here interface or the real class already" may be answered incorrectly). The best solution here I guess is when you will just provide PR.
Hi @msangel, thanks for the reply.
It's hard to do something without a clear understanding of all the requirements
I think it's very simple. The goal is just to be able to extend this library with Tags and Filters via an interface rather than an inheritance approach. That's it. This is just typical OO stuff. Let me know if that doesn't make sense, but I assume you have a handle on how interfaces work :)
should be here interface or the real class already
Sorry, but I'm not following that sentence. If you're asking where in the Liqp code you'd type via the interface instead of the abstract class type, I'm fairly sure the answer would be "everywhere". As in all instance of a method argument or variable declaration that currently uses Tag or Filter as the type, would be converted to ITag and IFilter (Or whatever you wished for the interfaces to be called).
The best solution here I guess is when you will just provide PR.
I'm not against that, but I don't have time at the moment. I also don't have a huge pressing need for this library-- I came across it and thought, "Hey that could be cool, but unfortunately I couldn't use it at this point" so the impetus for me to spend time on this is also fairly low. I'll keep this on the back burner and see if I have some time later if I do end up needing the library. In the mean time, I'm happy to help explain further what changes are needed. I really think it's just
- Create two interfaces
- Change the existing abstract
TagandFilterclass to implement those interfaces - Do a find/replace on the code to swap out the types
I have reviewed this once again and had to admit - using this will cause a lot of code compatibility breaking and most important will create unnecessary interfaces in the inheritance tree with no clear purpose. That already is quite big. Also this.
What I propose instead is create three new classes that will take interface as a parameter and uses one as rendering delegate. As simple as:
public interface InsertionDelegate {
/**
* Define the name of this insertion.
*/
String getName();
/**
* @see Insertion#render(TemplateContext, LNode...)
*/
Object render(TemplateContext context, LNode... nodes);
}
public class DelegatingTag extends Tag {
private final InsertionDelegate delegate;
public DelegatingTag(InsertionDelegate delegate) {
super(delegate.getName());
this.delegate = delegate;
}
@Override
public Object render(TemplateContext context, LNode... nodes) {
return delegate.render(context, nodes);
}
}
And others:
-
DelegatingFilter(DelegatingFilterDelegate) -
DelegatingTag(InsertionDelegate) -
DelegatingBlock(InsertionDelegate)
where all the constgructor parameters are insterfaces, and also static method creators:
-
DelegatingFilter.createFilter(DelegatingFilterDelegate) -
DelegatingInsertion.createTag(InsertionDelegate) -
DelegatingInsertion.createBlock(InsertionDelegate).
See code and sample here: https://github.com/bkiers/Liqp/commit/6ff1d496823342b3a2b586f67f82e81a8bdb43a8
// given
Insertion tag = DelegatingInsertion.createTag(new InsertionDelegate() {
@Override
public String getName() {
return "current_date";
}
@Override
public Object render(TemplateContext context, LNode... nodes) {
return ZonedDateTime.of(2000, 1, 1, 15, 0, 0, 0, ZoneOffset.UTC);
}
});
// when
String res = Template.parse("today is: {% current_date %}", new ParseSettings.Builder()
.with(tag)
.build()).render();
// then
assertEquals("today is: 2000-01-01 15:00:00 Z", res);
@bdw429s will this work for you?
using this will cause a lot of code compatibility breaking
Hmm, I'm not sure we're on the same page then. What I described above should not break any compatibility. Any existing subclass of Tag for example, would automatically implement the ITag interface after my proposed change, so the type would still be accepted by the framework with no changes necessary. What scenarios are you seeing break here?
and most important will create unnecessary interfaces in the inheritance tree with no clear purpose.
Again. I'm not sure what you mean here. The Interface is necessary as described in this ticket and the purpose seems rather clear. Are you concerned that people may look at the source code, see an interface called ITag and not understand its purpose? Because it seems like it would be abundantly obvious to me. Your solution still requires the creation of an interface, so it's the same amount of work, but actually extra work because you have an interface AND a delegate. I'm unclear why my interface would have no clear purpose but yours is somehow different.
What I propose instead is create three new classes that will take interface as a parameter and uses one as rendering delegate. will this work for you?
Probably, though to be honest, this approach feels far more circuitous to me, especially if you are concerned about "unnecessary" classes that don't have a clear purpose. Creating a delegate that extends the base class and accepts an interface will work, it just seems more complicated than switching to a backwards-compatible interface that can be passed directly to all the existing classes.
This is a classical case described in this article: https://en.wikipedia.org/wiki/Composition_over_inheritance
Also, when I add 2 interfaces and 3 classes, it affects only 5 logical elements.
Adding an intermediate interface will cause affecting all filters, tags and blocks. Literally, it will affect ~95 entities just in this library(by causing them also extend some new interface additionally) and will cause a few dozens of methods signature change. With my approach no methods interface would even change a bit. Can you feel the difference now?
You still can provide PR with your solution.
Also, using interfaces with ISomething prefix is bad practice that came from C# world and is not welcomed in java.
What speaks against making Tag/Block interfaces is the fact that these classes actually don't implement any new methods other than what's defined in their Insertion superclass. liqp uses these classes to differentiate between tags and blocks via instanceof. If they were interfaces, a subclass could implement both "ITag" and "IBlock" (for lack of a better name), which breaks this core assumption.
I'm inclined to close this was "wont fix". I think the delegate solution provided by @msangel is a sensible workaround if interfaces are truly needed.
@kohlschuetter If the Tag and Block classes have the same API has Insertion, then simply make an Insertion interface and be done with it! As far as "core assumption", I'd question if your assumption was safe to make. But really, I don't care- I just wanted an easy way to extend this library from another JVM language which is NOT JAVA and allows me to cloak CFML classes as a Java interface to easily pass my classes to any Java library that uses interface-based typing. I gave up a long time ago getting anywhere with the maintainer here. His delegate idea sounded far over complicated and even though I presented the clear use case several times, he kept repeating he didn't understand the use case. I moved on a long time ago from trying to use this library :/
I'm not going to defend any design decisions made by the original authors/maintainers, it's just that the API is the way it currently is: despite having the same methods, tags and blocks are just handled differently (blocks have an end tag), and, categorically, you cannot just introduce interfaces above the existing hierarchy without introducing some kind of identification as to how an implementing class going to be handled, either as tag, block or filter. We would have to make changes to the API, potentially alienating a few more people. Perhaps we could add "marker" methods to the current subclasses, but that is just kicking the can down the road. It can be done but I, too, question the actual benefit, especially when the delegate workaround is available to you.
Anyhow, thanks for replying even though you've moved on to something else already.
I, too, question the actual benefit,
You guys are insufferable. I provided the benifit in my original ticket description, only to have it ignored over and over. It was very simple-- to allow extension by custom classes provided by implementation of an interface, rather than extension of a base class (which requires the code to be in Java). It's quite simple and it blows my mind how may times I've repeated it only to be told there is no point.
We would have to make changes to the API
I don't agree with this and I outlined in the ticket above how it could be backwards compatible. However, it seemed the maintainer and I were always somehow on totally different pages so I gave up.
If the API has a method
registerWidget( FooBar widget )
and your custom widgets extend FooBar base class, then introducing an interface IFooBar and modifying FooBar to extend it would allow you to change the API method to this
registerWidget( IFooBar widget )
and all subclasses of FooBar would still inherently satisfy the contract since they would implement IFooBar via their extension of Foobar, thus having backwards compatibility.
But now with the added benifit that I would be able to provide a widget which implemented IFooBar but didn't necessarily use the FooBar base class (which is not easy to do in my JVM language).
And yes, if you have some code that received an Object and you need to test it, you'd need to check to see if it's an instance of IFoobar the interface, not FooBar the class, but I'd say that's a pretty easy change to make for the benifit of not forcing the users of the library to extend a Java class. I would choose interfaces over base classes any day of the week because of the greater flexibility it affords, and in my many years of writing CFML sdks for Java libraries and even the JDK, I can say interfaces are far more common for me to see used for extensibility.
when the delegate workaround is available to you.
But it isn't available! It was a counter-suggestion to my ticket, which quite frankly seemed over-complicated and unecessary. But nothing was ever implemented, so there is nothing "available" to me :)
You guys are insufferable
@bdw429s, I think you need to step back a little and tune down your language.
when the delegate workaround is available to you. But it isn't available! It was a counter-suggestion to my ticket, which quite frankly seemed over-complicated and > unecessary. But nothing was ever implemented, so there is nothing "available" to me :)
You haven't provided a pull request with concrete changes, which would have made a discussion more concrete than what you're trying to right now. In contrast, @msangel has provided you with a concrete solution that you can implement in your codebase right now.
Quite frankly, as a general rule, and this not only applies to you and this request, the chances of achieving what one wants correlates strongly with how one communicates. Right now, it seems you're just demanding and even insulting a group of volunteers of an open source project. You've just managed to lower my very own interest in addressing this issue to 0%.
You guys are insufferable
@bdw429s, I think you need to step back a little and tune down your language.
when the delegate workaround is available to you. But it isn't available! It was a counter-suggestion to my ticket, which quite frankly seemed over-complicated and > unecessary. But nothing was ever implemented, so there is nothing "available" to me :)
You haven't provided a pull request with concrete changes, which would have made a discussion more concrete than what you're trying to right now. In contrast, @msangel has provided you with a concrete solution that you can implement in your codebase right now.
Quite frankly, as a general rule, and this not only applies to you and this request, the chances of achieving what one wants correlates strongly with how one communicates. Right now, it seems you're just demanding and even insulting a group of volunteers of an open source project. You've just managed to lower my very own interest in addressing this issue to 0%.
Well said @kohlschuetter
tune down your language.
I'm sorry if that came across too strongly. But I stand by my comments as a description of how frustrating this conversation has been for me. Specifically, feeling like I'm not being listened to. To be told repeatedly that there is no benifit to my suggestion after explaining the benifit and the use case several times is difficult.
You haven't provided a pull request
You are completely correct. As an open source maintainer and contributor for the last 15+ years, I'm smart enough to not waste my time writing code that has no chance of being merged. I've sent way too many pulls to a project only to realize the maintainers didn't really want it and weren't going to merge it. It was clear no agreement was ever reached on an appropriate way to implement this, so I would have been foolish to send an unwanted pull.
provided you with a concrete solution that you can implement in your codebase right now.
Remember my frustration about not being listened to? You're doing it again. As I've said several times, I was not using Java, but a JIT JVM language which does not allow runtime extension of Java classes. The code provided is NOT something I can use-- at least not directly. It would need to be part of the core, which is what I said above. And to be quite clear, I'm not complaining that it hasn't been implemented. I was simply correcting you when you said it had.
you're just demanding
I have not demanded anything. You misrepresent me. As I said, I no longer have the need for this library, so the only reason for continuing to explain the ticket is the fact I feel this is still a useful and beneficial thing for the library to support (and I hate being misunderstood) I haven't complained one bit about the ticket not being done, only that there seems to be a repeated lack of understanding of what is being suggested and what fixes would have actually worked.
even insulting
Again, I apologize if you felt that way about it. I would suggest some introspection into my experience here and how being ignored repeatedly made this discussion frustrating. I believe I should be within my bounds to express that without you crying foul.
You've just managed to lower my very own interest in addressing this issue to 0%.
You seem to think you're hurting me here. Again, I've moved on, but still believed in the value of the enhancement. You not doing it only hurts your other potential users.
Despite the way I've apparently come across, I've enjoyed having the discussion (despite the frustrations) and I have similar conversations with my users in regards to the open source projects I maintain. The key I've found is to strive to understand the use case the users have and how your idea may not actually satisfy it. I wish you all well.