Velocity code style update

Development
21

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

  • +1: vmassol

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
22

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
23

@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 :slight_smile:

Have a good day,
Lucas.

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.

25

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
26

Looks good to me.

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!

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.

28

@KebabRonin I’ve checked the draft page, looks good to me too. Thanks for the work ! :slight_smile:

+1

For me, yes and it’s already what we use AFAIK.

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

29

Looks good to me, thanks for the time spent on it. Would probably be much easier to discuss each rule separately next time :slight_smile: