Re: [ANNOUNCE] Velocity Engine 2.1 test build available
On Tue, Mar 5, 2019 at 2:12 PM Claude Brisson wrote: > > Oh, you are referring to the recent commit #1854695, "[engine] Invalid > method calls should definitely be logged". > > Strict mode is not concerned here, its purpose is to throw exceptions > that will stop merging whenever an invalid reference or method is > encountered. > > Without strict mode, we were previously getting a debug log for null > references rendering, but nothing so far for invalid methods, that's the > problem this commit is addressing. > > But you are right, there *is* an inconsistency: the null references > message is at the DEBUG loglevel, and the new invalid reference message > is at the WARN level. > > So I'm gonna lower the latter one to DEBUG to be consistent. Thanks ! Apart from this it looks good, all the XWiki Velocity related unit test I had created for the problems I previous found with Velocity 2.0 are passing. Great job ! > >-- >Claude > > > On 04/03/2019 12:49, Thomas Mortagne wrote: > > Actually it's very clear why it's done like this sorry. > > > > What is really not clear and not really documented in the migration notes > > is why did this became a problem ? I can think of several use cases where > > not existing may be called and then a fallback done depending on the > > version of XWiki > > > > Le lun. 4 mars 2019 à 12:36, Thomas Mortagne a > > écrit : > > > >> I did not double checked but I don't remember this to be the case in > >> my previous quick tests with Velocity 2.0: I get > >> > >> [WARN] Object 'java.lang.String' does not contain method tyty() at > >> mytemplate[line 1, column 36] > >> > >> warnings despite the fact that I did not modified > >> "runtime.references.strict" default. > >> > >> The warnings I get are coming from > >> > >> http://svn.apache.org/viewvc/velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTMethod.java?view=markup#l179 > >> while I can see in debug mode that > >> > >> http://svn.apache.org/viewvc/velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/util/ClassUtils.java?view=markup#l220 > >> have the expected behavior (it does not enter the if and the boolean > >> is based on "runtime.references.strict" property). It seems to just be > >> missing a if on strictRef value. > >> > >> Tell me if I should create an issue on > >> https://issues.apache.org/jira/browse/VELOCITY. > >> > >> On Mon, Mar 4, 2019 at 2:47 AM Claude Brisson > >> wrote: > >>> > >>> On 03/03/2019 21:52, Claude Brisson wrote: > > > > Is this list complete? > > http://velocity.apache.org/engine/2.0/configuration.html > Apart from 2.1 novelties, I'm quite sure, yes. All new ones are > visible in http://velocity.apache.org/engine/devel/configuration.html > >>> It looks like this one is an orphan and can be removed: > >>> > >>> /** Switch for VM blather: default true. */ > >>> String VM_MESSAGES_ON = "velocimacro.messages.on"; > >>> > >>> But it also appears that "userdirective" is absent from > >>> RuntimeConstants, as well as from the documentation... since a long time. > >>> > >>> > >>> > >> > >> -- > >> Thomas Mortagne > >> > > - > To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org > For additional commands, e-mail: dev-h...@velocity.apache.org > -- Thomas Mortagne - To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org For additional commands, e-mail: dev-h...@velocity.apache.org
Re: [ANNOUNCE] Velocity Engine 2.1 test build available
Oh, you are referring to the recent commit #1854695, "[engine] Invalid method calls should definitely be logged". Strict mode is not concerned here, its purpose is to throw exceptions that will stop merging whenever an invalid reference or method is encountered. Without strict mode, we were previously getting a debug log for null references rendering, but nothing so far for invalid methods, that's the problem this commit is addressing. But you are right, there *is* an inconsistency: the null references message is at the DEBUG loglevel, and the new invalid reference message is at the WARN level. So I'm gonna lower the latter one to DEBUG to be consistent. -- Claude On 04/03/2019 12:49, Thomas Mortagne wrote: Actually it's very clear why it's done like this sorry. What is really not clear and not really documented in the migration notes is why did this became a problem ? I can think of several use cases where not existing may be called and then a fallback done depending on the version of XWiki Le lun. 4 mars 2019 à 12:36, Thomas Mortagne a écrit : I did not double checked but I don't remember this to be the case in my previous quick tests with Velocity 2.0: I get [WARN] Object 'java.lang.String' does not contain method tyty() at mytemplate[line 1, column 36] warnings despite the fact that I did not modified "runtime.references.strict" default. The warnings I get are coming from http://svn.apache.org/viewvc/velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTMethod.java?view=markup#l179 while I can see in debug mode that http://svn.apache.org/viewvc/velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/util/ClassUtils.java?view=markup#l220 have the expected behavior (it does not enter the if and the boolean is based on "runtime.references.strict" property). It seems to just be missing a if on strictRef value. Tell me if I should create an issue on https://issues.apache.org/jira/browse/VELOCITY. On Mon, Mar 4, 2019 at 2:47 AM Claude Brisson wrote: On 03/03/2019 21:52, Claude Brisson wrote: Is this list complete? http://velocity.apache.org/engine/2.0/configuration.html Apart from 2.1 novelties, I'm quite sure, yes. All new ones are visible in http://velocity.apache.org/engine/devel/configuration.html It looks like this one is an orphan and can be removed: /** Switch for VM blather: default true. */ String VM_MESSAGES_ON = "velocimacro.messages.on"; But it also appears that "userdirective" is absent from RuntimeConstants, as well as from the documentation... since a long time. -- Thomas Mortagne - To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org For additional commands, e-mail: dev-h...@velocity.apache.org
Re: [ANNOUNCE] Velocity Engine 2.1 test build available
Actually it's very clear why it's done like this sorry. What is really not clear and not really documented in the migration notes is why did this became a problem ? I can think of several use cases where not existing may be called and then a fallback done depending on the version of XWiki Le lun. 4 mars 2019 à 12:36, Thomas Mortagne a écrit : > I did not double checked but I don't remember this to be the case in > my previous quick tests with Velocity 2.0: I get > > [WARN] Object 'java.lang.String' does not contain method tyty() at > mytemplate[line 1, column 36] > > warnings despite the fact that I did not modified > "runtime.references.strict" default. > > The warnings I get are coming from > > http://svn.apache.org/viewvc/velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTMethod.java?view=markup#l179 > while I can see in debug mode that > > http://svn.apache.org/viewvc/velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/util/ClassUtils.java?view=markup#l220 > have the expected behavior (it does not enter the if and the boolean > is based on "runtime.references.strict" property). It seems to just be > missing a if on strictRef value. > > Tell me if I should create an issue on > https://issues.apache.org/jira/browse/VELOCITY. > > On Mon, Mar 4, 2019 at 2:47 AM Claude Brisson > wrote: > > > > > > On 03/03/2019 21:52, Claude Brisson wrote: > > >> > > >> > > >> Is this list complete? > > >> http://velocity.apache.org/engine/2.0/configuration.html > > > > > > Apart from 2.1 novelties, I'm quite sure, yes. All new ones are > > > visible in http://velocity.apache.org/engine/devel/configuration.html > > > > It looks like this one is an orphan and can be removed: > > > > /** Switch for VM blather: default true. */ > > String VM_MESSAGES_ON = "velocimacro.messages.on"; > > > > But it also appears that "userdirective" is absent from > > RuntimeConstants, as well as from the documentation... since a long time. > > > > > > > > > -- > Thomas Mortagne >
Re: [ANNOUNCE] Velocity Engine 2.1 test build available
I did not double checked but I don't remember this to be the case in my previous quick tests with Velocity 2.0: I get [WARN] Object 'java.lang.String' does not contain method tyty() at mytemplate[line 1, column 36] warnings despite the fact that I did not modified "runtime.references.strict" default. The warnings I get are coming from http://svn.apache.org/viewvc/velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTMethod.java?view=markup#l179 while I can see in debug mode that http://svn.apache.org/viewvc/velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/util/ClassUtils.java?view=markup#l220 have the expected behavior (it does not enter the if and the boolean is based on "runtime.references.strict" property). It seems to just be missing a if on strictRef value. Tell me if I should create an issue on https://issues.apache.org/jira/browse/VELOCITY. On Mon, Mar 4, 2019 at 2:47 AM Claude Brisson wrote: > > > On 03/03/2019 21:52, Claude Brisson wrote: > >> > >> > >> Is this list complete? > >> http://velocity.apache.org/engine/2.0/configuration.html > > > > Apart from 2.1 novelties, I'm quite sure, yes. All new ones are > > visible in http://velocity.apache.org/engine/devel/configuration.html > > It looks like this one is an orphan and can be removed: > > /** Switch for VM blather: default true. */ > String VM_MESSAGES_ON = "velocimacro.messages.on"; > > But it also appears that "userdirective" is absent from > RuntimeConstants, as well as from the documentation... since a long time. > > > -- Thomas Mortagne - To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org For additional commands, e-mail: dev-h...@velocity.apache.org
Re: [ANNOUNCE] Velocity Engine 2.1 test build available
On 03/03/2019 21:52, Claude Brisson wrote: Is this list complete? http://velocity.apache.org/engine/2.0/configuration.html Apart from 2.1 novelties, I'm quite sure, yes. All new ones are visible in http://velocity.apache.org/engine/devel/configuration.html It looks like this one is an orphan and can be removed: /** Switch for VM blather: default true. */ String VM_MESSAGES_ON = "velocimacro.messages.on"; But it also appears that "userdirective" is absent from RuntimeConstants, as well as from the documentation... since a long time.
Re: [ANNOUNCE] Velocity Engine 2.1 test build available
On 03/03/2019 20:09, Michael Osipov wrote: I have noticed that too actually. I always wondered about this quirk: resource.loader = file,classpath,etc. file.resource.loader.path = ... instead of resource.loaders = file,classpath resource.loader.file.path = More standard, I agree. Is this list complete? http://velocity.apache.org/engine/2.0/configuration.html Apart from 2.1 novelties, I'm quite sure, yes. All new ones are visible in http://velocity.apache.org/engine/devel/configuration.html Claude - To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org For additional commands, e-mail: dev-h...@velocity.apache.org
Re: [ANNOUNCE] Velocity Engine 2.1 test build available
Am 2019-03-03 um 19:29 schrieb Claude Brisson: Thanks for the diligent feedback. Going through the notes makes make stumble upon this: Added a new 'parser.allows.dash.in.identifiers' boolean property (false per default) to (dis)allow '-' in reference identifiers . Fixes VELOCITY-542. I see a few issues here: * The property name is awkward. Where is the hierarchy? Shouldn't it rather be 'parser.allow_dash_in_identifiers'? (Applies to VELOCITY-904 too). * '-' U+002D is *not* a dash. It is a hyphen (minus). A lot of people get and do it wrong. I would strongly recommend to fix that typographic issue /before/ 2.1. So the new property should be named "parser.allow_hyphen_in_identifiers". Looking at the current state of our runtime properties names, I see that there are many inconsistencies: - '.' is used many times as a word separator instead of a hierarchical separator - sometimes there is no word separator at all, as in "resource.manager.logwhenfound" - sometimes we use CamelCase, as in "resource.loader.modificationCheckInterval" I have noticed that too actually. I always wondered about this quirk: resource.loader = file,classpath,etc. file.resource.loader.path = ... instead of resource.loaders = file,classpath resource.loader.file.path = Maybe it's time to homogenize all this. Old property names will of course still be available via a deprecation mechanism. Absolutely, yes. Deprecate in 2.1, replace in 3.0. Any preference between underscore_word_separator and CamelCase? My general rule of thumb is: if you have more than one element beneath, use the period, e.g, if there is sothing else besides the 'loader' in 'resource' otherwise it should be resource_loader. I do not have any strong prefecence of either format, but I think that snake case (with underscores) and periods reads better. Regardless which pattern will be applied, it has to be consistent throughout. Is this list complete? http://velocity.apache.org/engine/2.0/configuration.html Michael - To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org For additional commands, e-mail: dev-h...@velocity.apache.org
Re: [ANNOUNCE] Velocity Engine 2.1 test build available
Thanks for the diligent feedback. Going through the notes makes make stumble upon this: Added a new 'parser.allows.dash.in.identifiers' boolean property (false per default) to (dis)allow '-' in reference identifiers . Fixes VELOCITY-542. I see a few issues here: * The property name is awkward. Where is the hierarchy? Shouldn't it rather be 'parser.allow_dash_in_identifiers'? (Applies to VELOCITY-904 too). * '-' U+002D is *not* a dash. It is a hyphen (minus). A lot of people get and do it wrong. I would strongly recommend to fix that typographic issue /before/ 2.1. So the new property should be named "parser.allow_hyphen_in_identifiers". Looking at the current state of our runtime properties names, I see that there are many inconsistencies: - '.' is used many times as a word separator instead of a hierarchical separator - sometimes there is no word separator at all, as in "resource.manager.logwhenfound" - sometimes we use CamelCase, as in "resource.loader.modificationCheckInterval" Maybe it's time to homogenize all this. Old property names will of course still be available via a deprecation mechanism. Any preference between underscore_word_separator and CamelCase? Other issues: * "Fix parsing of a terminal hash or dollar sign in sing litteral and template" should rather be "...sign literal..."? * *The minimum JDK is now 1.8*: This should have been a separate issue and stated right on the front page for our users instead of being a side note. Very true. Claude - To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org For additional commands, e-mail: dev-h...@velocity.apache.org
Re: [ANNOUNCE] Velocity Engine 2.1 test build available
Am 2019-03-03 um 16:17 schrieb Claude Brisson:
The test build of Velocity Engine 2.1 is available.
No determination as to the quality ('alpha,' 'beta,' or 'GA') of
Velocity Engine 2.1 has been made, and at this time it is simply a "test
build". We welcome any comments you may have, and will take all feedback
into account if a quality vote is called for this build.
Release notes:
*
https://dist.apache.org/repos/dist/dev/velocity/velocity-engine/2.1/release-notes.html
Thanks for the effort Claude!
Going through the notes makes make stumble upon this:
Added a new 'parser.allows.dash.in.identifiers' boolean property (false
per default) to (dis)allow '-' in reference identifiers . Fixes
VELOCITY-542.
I see a few issues here:
* The property name is awkward. Where is the hierarchy? Shouldn't it
rather be 'parser.allow_dash_in_identifiers'? (Applies to VELOCITY-904 too).
* '-' U+002D is *not* a dash. It is a hyphen (minus). A lot of people
get and do it wrong. I would strongly recommend to fix that typographic
issue /before/ 2.1.
Other issues:
* "Fix parsing of a terminal hash or dollar sign in sing litteral and
template" should rather be "...sign literal..."?
* *The minimum JDK is now 1.8*: This should have been a separate issue
and stated right on the front page for our users instead of being a side
note.
Michael
-
To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org
