attaching the patch for review/comments.

one more small patch after fully testing the changes for extensibility

Thanks Aaron. Some comments:

1. DateFormatEvaluator methods such as evaluateWrapper, evaluateString, parseMathString, resolveMapper and getDateMathParser have no business being public but we can make them protected if you want. All of them should be marked as experimental API.
2. Evaluator.parseParams should be protected not public.
3. I don't like that we are creating a method such as getVariableWrapper. These things are not supposed to be pluggable and it should definitely not be public. We can mark it as protected along with a javadoc warning saying that this is experimental API. If I were writing Evaluator today, I'd just use a Callable instead.

On an unrelated note, I wonder why we cache all available locales and timezones. If looking up locale/timezone is expensive then it can be done as-needed. I'll create an issue.

Thanks Shalin,

I'm attaching another patch to change the method accessors to protected (instead of public) and marked the methods as lucene.experimental. Let me know if there's anything else. Otherwise, can you, or someone else commit/push these patches into the 4.x branch so it makes the next release?

Thanks

NOTE: the patches can be applied from oldest to newest

Thanks Aaron. Here's a single patch with all your changes. We typically attach a patch named as SOLR-xxx.patch and if/when we need to revise, we upload patch with the same name again. Jira will automatically gray out the older patches in the UI. This makes it easier to know the latest patch.

I made three changes:

1. FileListEntityProcessor.getDate was missing a return statement
2. I removed the getters for VariableWrapper and made the attributes public final
3. Changed DateFormatCacheKey to static protected.

I will commit this shortly.

Commit 1591573 from shalin@apache.org in branch 'dev/trunk'
[ https://svn.apache.org/r1591573 ]

SOLR-6013: Fix method visibility of Evaluator, refactor DateFormatEvaluator for extensibility

Commit 1591574 from shalin@apache.org in branch 'dev/branches/branch_4x'
[ https://svn.apache.org/r1591574 ]

SOLR-6013: Fix method visibility of Evaluator, refactor DateFormatEvaluator for extensibility

Thanks Aaron!

Thanks Shalin, but, you forgot two of the public 'getter' methods that I added in the last patch to Evaluator.VariableWrapper:

public String getVarName()

public VariableResolver getVariableResolver()

Can you fix/commit/close?
Thanks.

Aaron

you forgot two of the public 'getter' methods that I added in the last patch to Evaluator.VariableWrapper

No, I didn't. Those two are now public final fields.

Shalin, that's fine I suppose (sorry I didn't notice you changed them to public/final). I'm just wondering though, wouldn't it make sense to access the bean properties using traditional getter methods instead of accessing them directly? Just curious as to the reasoning of not providing the getters. In either case, I'm fine with whatever you decide and re-closing this issue.

Thanks.

Aaron

Shalin, that's fine I suppose (sorry I didn't notice you changed them to public/final). I'm just wondering though, wouldn't it make sense to access the bean properties using traditional getter methods instead of accessing them directly? Just curious as to the reasoning of not providing the getters. In either case, I'm fine with whatever you decide and re-closing this issue.

This is just a simple internal object with final values. There is no value added by getters here.