SLICE-128 making SliceLookUpTag type attribute value String #109
Conversation
|
| } finally { | ||
| clean(); | ||
| } | ||
| } | ||
|
|
||
| public void setType(Class<?> type) { | ||
| public void setType(String type) { |
There was a problem hiding this comment.
Changing the signature of this method forces you to export the package in version 5.0.0. We're not quite there to release Slice 5. Maybe we can add just new method - the should allow this package to be in version 4.4.0.
There was a problem hiding this comment.
@mmajchrzak i'm not able to overload setType with String argument here. I think overloading is not supported in tag lib. do you want me to create a new attribute altogether ?
There was a problem hiding this comment.
If we keep this method with changed signature, then this code will be released in Slice 5. I'm not sure about overloading in taglib neither - this needs checking. To keep backward compatibility we could also think about introducing a new parameter, but I'd definitely prefer overloading if it's possible.
There was a problem hiding this comment.
I've did some searching and trials and errors with code as well, so far what I've got is overloading is not possible for taglibs, also mentioned on this blog.
I've also did a POC on having a separate 'name' attribute. It works for sure, but is not without it's own repercussions. What if both type and name attribute are used(and say for an edge case, both have different class values), which one should be preferred is the mystery?
Leaving it up to you, should i keep looking for a way to overload the taglib attribute setter or go with a new attribute.
There was a problem hiding this comment.
I'd double check the overloading - the blog post you referenced is mentioning JavaBeans not taglibs.
There was a problem hiding this comment.
Have a look at this documentation
Setting Deferred Value Attributes and Deferred Method Attributes section in particular.
It mentions that i can have __setAttr(Object obj) __ and then check instanceOf obj.
I can do a test run of this change, but firstly what are your views on this change ?
Will the major version of Slice needs to be incremented, if i update only the setter signature and not the actual attribute type ?
There was a problem hiding this comment.
Unfortunately yes, because the plugin which controls the version number looks at the method signatures. From my point of view, if we can't keep the current setter method, I'd postpone the implementation until the Slice 5, which is allowed to be break the API.
| * @return {@link Class} object pertaining to {@code type} as it's canonical name | ||
| * @throws ClassNotFoundException if the class was not found | ||
| */ | ||
| public static Class<?> getClassFromType(final PageContext pageContext, final String type) throws ClassNotFoundException { |
There was a problem hiding this comment.
@mmajchrzak I've also refactored this method to use ModelProvider class to get it's Class object.
Please have a look at this one as well.
There was a problem hiding this comment.
Hmm.. By introducing of this method you're fetching the model twice during tag execution. Refactor it please, so that modelProvider#get(String, Resource) is used but model is read only once.
|
@Shashi-Bhushan Please also include jira ticket number at the beginning of every commit. This makes the commit history easier to read. Thanks. |
…ring, String) and commented Test Case
Shashi-Bhushan
force-pushed
the
SLICE-128
branch
from
2688d84
to
391e3d0
Compare
|
@mmajchrzak Added a new Util method SliceTagUtils#getFromCurrentPath(PageContext, String, String), Also commented out the Failing Test case for now. Will include it after a final implementation of the method in SliceTagUtils. |
Shashi-Bhushan
force-pushed
the
SLICE-128
branch
from
b22f175
to
c676d8b
Compare
|
@mmajchrzak Additionally, I've been wanting to write a test case for slice:lookup tag test, using JspTagTrait. |
WIP