Since the discussion died down, I’ll be making a draft page for the proposal. Here is a summary of the opinions above:
1.1. Sentence case comments (## This is a comment.
)
+1: Marius, MichaelHamann, surli, vmassol (only for sentences)
+0: CharpentierLucas
1.2. Multiple single-line comments
+1: Marius, MichaelHamann, surli, vmassol (multi-line comment only for javadoc)
+1: CharpentierLucas (already in use)
1.3. Issues quoted as ID + TITLE
+1: Marius, MichaelHamann, surli, vmassol (id + title)
+0: CharpentierLucas (could cause problems when workarounds are spread among multiple files)
1.4. Javadoc comments for large velocity scripts
+1: Marius, MichaelHamann, surli, vmassol (multi-line comment, not multiple single line comments)
+0: CharpentierLucas (define what larger velocity script
means)
1.5. Trailing comments
-1: Marius, MichaelHamann, surli, vmassol
2.1. Space delimited = (#set ($a = $b)
)
+1: Marius, MichaelHamann, surli, vmassol, CharpentierLucas
2.2. ${a}
only when necessary
+1: Marius, MichaelHamann, surli, vmassol, CharpentierLucas
2.3. Space between parantheses (#set ($a = $f.method($c.get($d)) )
)
-1: Marius, MichaelHamann, surli, vmassol
-0: CharpentierLucas
2.3.1. Commas between parameters in macro calls
2.4. Space between directive and open bracket (#if ($a)
)
+1: Marius, MichaelHamann, surli, vmassol (only for velocity directives, not for defined macros)
2.5. Separate velocity macros for definitions and code
0: Marius, MichaelHamann, surli (best practice, not rule; add new line between macros)
0: CharpentierLucas (only new line, but how to handle when you need to define a macro inside of code?)
+0/-1: vmassol (give more examples, there can be cases where it impedes the code)
3.1. Other links
+1: CharpentierLucas
-1: vmassol
3.2. Meta-style page
-1: CharpentierLucas
0: vmassol (more precise proposals)
3.3. New page for velocity resources
+1: CharpentierLucas
0: vmassol (more precise proposals)
2 Likes
The draft page is completed. Please tell me what you think. You can edit the page directly if you have improvements/fixes. Here is the link again:
https://dev.xwiki.org/xwiki/bin/view/Drafts/Velocity%20Code%20Style
1 Like
@KebabRonin Looks good to me!
Do you want to move your Draft into the doc or should I handle it?
The doc is public, so if anyone still spots a mistake, they can still edit it directly
Have a good day,
Lucas.
vmassol
December 10, 2024, 12:59pm
24
Ì haven’t reviewed the draft page and idk if the content has been agreed by everyone. We can’t move it before that’s the case.
I tried to only include the suggestions with no negative votes.
@mflorea @surli @MichaelHamann Could you take another look if the draft is ok?
Another proposal:
Velocimacros which return values (or modify existing variables) should have those variables as part of their parameters. This makes it clear which variables will be changed by a macro, reducing the situations when a macro call has hidden side effects, which can be hard to debug in bigger scripts. Example:
## Not like this:
#macro (configureDisplayText $parameters)
#if ($parameters['user'])
#set ($displayText = 'user ' + $xwiki.getUser())
#else
#set ($displayText = 'unknown value')
#end
#set ($userFound = "$!parameters['user']" == '')
#end
## But like this:
#macro (configureDisplayText $parameters $displayText $userFound)
#if ($parameters['user'])
#set ($displayText = 'user' + $xwiki.getUser())
#else
#set ($displayText = 'unknown value')
#end
#set ($userFound = "$!parameters['user']" == '')
#end
mleduc
December 11, 2024, 8:05am
27
Always capture the result of a method call in a variable to avoid side effects
Do we want to make it explicit to always use the $discard
variable for the capture. And, to never use $discard
for anything else?
The rest looks good, thanks!
KebabRonin:
I tried to only include the suggestions with no negative votes.
@mflorea @surli @MichaelHamann Could you take another look if the draft is ok?
Another proposal:
Velocimacros which return values (or modify existing variables) should have those variables as part of their parameters. This makes it clear which variables will be changed by a macro, reducing the situations when a macro call has hidden side effects, which can be hard to debug in bigger scripts. Example:
## Not like this:
#macro (configureDisplayText $parameters)
#if ($parameters['user'])
#set ($displayText = 'user ' + $xwiki.getUser())
#else
#set ($displayText = 'unknown value')
#end
#set ($userFound = "$!parameters['user']" == '')
#end
## But like this:
#macro (configureDisplayText $parameters $displayText $userFound)
#if ($parameters['user'])
#set ($displayText = 'user' + $xwiki.getUser())
#else
#set ($displayText = 'unknown value')
#end
#set ($userFound = "$!parameters['user']" == '')
#end
Sounds good as well. I wonder if we shouldn’t introduce a naming convention for variables that will be used as return value. For instance, with the out_
prefix, e.g., $out_userFound
?
To make it clear which variable is only here as a return value.
vmassol
December 11, 2024, 11:19am
28
@KebabRonin I’ve checked the draft page, looks good to me too. Thanks for the work !
KebabRonin:
Velocimacros which return values (or modify existing variables) should have those variables as part of their parameters. This makes it clear which variables will be changed by a macro, reducing the situations when a macro call has hidden side effects, which can be hard to debug in bigger scripts. Example:
+1
For me, yes and it’s already what we use AFAIK.
mleduc:
Sounds good as well. I wonder if we shouldn’t introduce a naming convention for variables that will be used as return value. For instance, with the out_
prefix, e.g., $out_userFound
?
To make it clear which variable is only here as a return value.
Not sure about this. I’m hesitating. For sure we’ve not been using this so far. If we do this I wouldn’t use an _
but just a prefix (like $outUserFound
).
Looks good to me, thanks for the time spent on it. Would probably be much easier to discuss each rule separately next time