Skip to content
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

SLICE-128 making SliceLookUpTag type attribute value String #109

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Shashi-Bhushan
Copy link
Contributor

WIP

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 71.32% when pulling 48f58b4 on Shashi-Bhushan:SLICE-128 into fe0b2f7 on Cognifide:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 71.32% when pulling ee07da0 on Shashi-Bhushan:SLICE-128 into fe0b2f7 on Cognifide:master.

@Shashi-Bhushan
Copy link
Contributor Author

Slice-128

  • made type attribute in slice:lookup tag of String type

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 71.32% when pulling 314679a on Shashi-Bhushan:SLICE-128 into fe0b2f7 on Cognifide:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 71.365% when pulling 4b910c0 on Shashi-Bhushan:SLICE-128 into fe0b2f7 on Cognifide:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 71.81% when pulling 46e22c6 on Shashi-Bhushan:SLICE-128 into fe0b2f7 on Cognifide:master.

} finally {
clean();
}
}

public void setType(Class<?> type) {
public void setType(String type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd double check the overloading - the blog post you referenced is mentioning JavaBeans not taglibs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 71.81% when pulling 572e333 on Shashi-Bhushan:SLICE-128 into fe0b2f7 on Cognifide:master.

* @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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 71.094% when pulling 2688d84 on Shashi-Bhushan:SLICE-128 into fe0b2f7 on Cognifide:master.

@mmajchrzak
Copy link
Contributor

@Shashi-Bhushan Please also include jira ticket number at the beginning of every commit. This makes the commit history easier to read. Thanks.

@Shashi-Bhushan
Copy link
Contributor Author

@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.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 71.049% when pulling 391e3d0 on Shashi-Bhushan:SLICE-128 into fe0b2f7 on Cognifide:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 70.781% when pulling 3b39c6f on Shashi-Bhushan:SLICE-128 into 433f18b on Cognifide:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 70.781% when pulling 3b39c6f on Shashi-Bhushan:SLICE-128 into 433f18b on Cognifide:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 70.781% when pulling 3b39c6f on Shashi-Bhushan:SLICE-128 into 433f18b on Cognifide:master.

@Shashi-Bhushan
Copy link
Contributor Author

@mmajchrzak
I've updated SliceLookupTag.java. The setter can be now be used for String as well, but as you specified because major version has been incremented, this will probably go into master when it's time for 5.0 release.
Removed unnecessary Test case as well.

Additionally, I've been wanting to write a test case for slice:lookup tag test, using JspTagTrait.
Could not use JspTagTrait it being an interface in Prosper-3.0(it's a Groovy trait in later versions, apologies I've never used interface version of this). If you could point me towards how i can write a test case for this now, i would be happy to write the test case as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants