Bug 30002

It should be installed at the correct place and used by the IDE, as well as by the QA check that should exist to catch incorrect tidy.

QA check will be added by https://gitlab.com/koha-community/qa-test-tools/-/merge_requests/50

Should we reuse this bug to move xt/perltidyrc to .perltidyrc and modify it to match our current guidelines ?

Created attachment 149027 [details] [review]
Bug 30002: Add .perltidyrc

The options set in this file try to match the current code style without
deviating too much from perltidy's defaults.

Created attachment 149028 [details] [review]
Bug 30002: Remove unused xt/perltidyrc

(In reply to Julian Maurice from comment #3)
> Created attachment 149027 [details] [review] [review]
> Bug 30002: Add .perltidyrc
> 
> The options set in this file try to match the current code style without
> deviating too much from perltidy's defaults.

If you want to see the result of applying this .perltidyrc, you can take a look at that commit: https://gitlab.com/jajm/Koha/-/commit/bc17de38be3e2cc04d316583996c971e7faa20a8

I would extend the max line length

--maximum-line-length=178

(In reply to Jonathan Druart from comment #6)
> I would extend the max line length
> 
> --maximum-line-length=178

I agree, but couldn't decide on a specific number, and I did not want to start a war on "the best max line length" so I kept the default.

There is also the option --variable-maximum-line-length than can be tried.

What is the status of this?

This need to be reviewed by QA team, so... Signed Off ?

The current max line length is too short, and 178 is too much in my testing.

For instance I'd like to see

my $h = { one => 1, two => 3, };

always on 2 lines

my $h = {
    one => 1,
    two => 3,
};

Otherwise 178 leads to hard to read structure.

Have you tried --break-at-old-comma-breakpoints ?
With this flag it should keep the line breaks, no matter what the max line length is.

--comma-arrow-breakpoints=0 appears to solve the issue as well.

I can see why Julian didn't extend the line length.. I thought I'd have a go with the 178 suggestion and didn't like it at all.

For me, I max out at around 120 for preference.  Reasoning behind that for me is that I often read code on a widescreen monitor in portrait orientation. and at my comfortable font size 120 chars at (my term actually fits 128, but I don't mind the small right margin.. 120 happens to match what the mojolicious project uses). This also allows me to put two windows side by side on a wide screen and not have my lines get wrapped.

How to move this forward the best?

I think we need to decide on line length, does 120 sound like a reasonable compromise?

I will be on it, I just need more time...

Discussion at the 26 April 2023 Development IRC Meeting.

To vote on this at the next Development IRC Meeting - see https://meetings.koha-community.org/2023/development_irc_meeting_26_april_2023.2023-04-26-14.31.log.html#l-87

Added to the agenda: https://wiki.koha-community.org/wiki/Development_IRC_meeting_10_May_2023

(In reply to David Nind from comment #16)
> Discussion at the 26 April 2023 Development IRC Meeting.
> 
> To vote on this at the next Development IRC Meeting - see
> https://meetings.koha-community.org/2023/
> development_irc_meeting_26_april_2023.2023-04-26-14.31.log.html#l-87
> 
> Added to the agenda:
> https://wiki.koha-community.org/wiki/Development_IRC_meeting_10_May_2023

There is nothing to vote on, I am going to work on it. Please wait a bit more here.

Created attachment 150517 [details] [review]
Bug 30002: Adjust perltidy

Remove the following 3 options that are not available in perltidy
v20190601 that is the version shipped with bullseye
--add-terminal-newline
--valign-exclusion-list
--extended-continuation-indentation

Extend max line length
--maximum-line-length=120

Prevent perltidy to format lists (see bug 30002 comment 10)
--break-at-old-comma-breakpoints

We could eventually go with this version, merge the QA checks, and see how it goes.

Given that we are not doing a tidy on the whole codebase we could refine it later if needed.

There are some parts that are uglier than before, for instance the following block in Koha/Patron.pm

335                 if ( C4::Context->preference("BorrowersLog") ) {

But globally it's nicer!

(In reply to Jonathan Druart from comment #18)
> Remove the following 3 options that are not available in perltidy
> v20190601 that is the version shipped with bullseye
> --add-terminal-newline
> --valign-exclusion-list
> --extended-continuation-indentation

That's something I haven't considered : different versions of perltidy will generate different code. For instance, vertical alignment of qw() after use is on by default since v20220613.
Should we force usage of a specific version then ? Maybe add a version check in the QA scripts too.

Also, since it's a developer-only tool, do we need to restrict ourselves to an old version ?

(P.S. That doesn't change anything, but bullseye's version of perltidy is v20200110, not v20190601)

(In reply to Julian Maurice from comment #21)
> (P.S. That doesn't change anything, but bullseye's version of perltidy is
> v20200110, not v20190601)

Indeed, it was on Ubuntu 20.04!

(In reply to Julian Maurice from comment #21)
> Also, since it's a developer-only tool, do we need to restrict ourselves to
> an old version ?

We could package the latest version on CPAN and have it in our repo.

Mason, would that work for you?

Suggestion addition for the wiki at https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL1:_Perltidy

"""
The code must be tidy using perltidy and the .perltidyrc file presents at the root of the Koha directory.
You must run perltidy on any new perl files added.
When you add changes to existing files you should consider setting your code editor accordingly.
For vim you can add the following line to you `.vimrc`: `vmap <F8> :!perltidy -q<CR>`
Then switch to visual move (ctrl+v), select a block and press F8.
"""

Please consider this as a candidate for 23.11 early pushes.

Created attachment 152161 [details] [review]
Bug 30002: Add .perltidyrc

The options set in this file try to match the current code style without
deviating too much from perltidy's defaults.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 152162 [details] [review]
Bug 30002: Remove unused xt/perltidyrc

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 152163 [details] [review]
Bug 30002: Adjust perltidy

Remove the following 3 options that are not available in perltidy
v20190601 that is the version shipped with bullseye
--add-terminal-newline
--valign-exclusion-list
--extended-continuation-indentation

Extend max line length
--maximum-line-length=120

Prevent perltidy to format lists (see bug 30002 comment 10)
--break-at-old-comma-breakpoints

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Pushed to master for 23.11.

Nice work everyone, thanks!

(In reply to Jonathan Druart from comment #24)
> Suggestion addition for the wiki at
> https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL1:_Perltidy
> 
> """
> The code must be tidy using perltidy and the .perltidyrc file presents at
> the root of the Koha directory.
> You must run perltidy on any new perl files added.
> When you add changes to existing files you should consider setting your code
> editor accordingly.
> For vim you can add the following line to you `.vimrc`: `vmap <F8>
> :!perltidy -q<CR>`
> Then switch to visual move (ctrl+v), select a block and press F8.
> """

Added to the wiki.

(In reply to Jonathan Druart from comment #30)
> (In reply to Jonathan Druart from comment #24)
> > Suggestion addition for the wiki at
> > https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL1:_Perltidy
> > 
> > """
> > The code must be tidy using perltidy and the .perltidyrc file presents at
> > the root of the Koha directory.
> > You must run perltidy on any new perl files added.
> > When you add changes to existing files you should consider setting your code
> > editor accordingly.
> > For vim you can add the following line to you `.vimrc`: `vmap <F8>
> > :!perltidy -q<CR>`
> > Then switch to visual move (ctrl+v), select a block and press F8.
> > """
> 
> Added to the wiki.

I added the VSCode counterpart.

(In reply to Tomás Cohen Arazi from comment #31)
> (In reply to Jonathan Druart from comment #30)
> > (In reply to Jonathan Druart from comment #24)
> > > Suggestion addition for the wiki at
> > > https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL1:_Perltidy
> > > 
> > > """
> > > The code must be tidy using perltidy and the .perltidyrc file presents at
> > > the root of the Koha directory.
> > > You must run perltidy on any new perl files added.
> > > When you add changes to existing files you should consider setting your code
> > > editor accordingly.
> > > For vim you can add the following line to you `.vimrc`: `vmap <F8>
> > > :!perltidy -q<CR>`
> > > Then switch to visual move (ctrl+v), select a block and press F8.
> > > """
> > 
> > Added to the wiki.
> 
> I added the VSCode counterpart.

Updated the general part of the page and Geany.

Does anything need to be done for folk who code in koha-testing-docker using vim?

(In reply to Jonathan Druart from comment #23)
> (In reply to Julian Maurice from comment #21)
> > Also, since it's a developer-only tool, do we need to restrict ourselves to
> > an old version ?
> 
> We could package the latest version on CPAN and have it in our repo.
> 
> Mason, would that work for you?

sure, i've added the latest perltidy (20230309) to the dev/unstable/master suite in koha-staging repo 

if it tests ok, ill push it everywhere


# apt policy perltidy
perltidy:
  Installed: 20230309-1~koha1
  Candidate: 20230309-1~koha1
  Version table:
 *** 20230309-1~koha1 500
        500 http://debian.koha-community.org/koha-staging dev/main amd64 Packages
        100 /var/lib/dpkg/status
     20220613-1 500
        500 http://ftp.nz.debian.org/debian sid/main amd64 Packages

(In reply to David Cook from comment #33)
> Does anything need to be done for folk who code in koha-testing-docker using
> vim?

read comment 24?

(In reply to Mason James from comment #34)
> (In reply to Jonathan Druart from comment #23)
> > (In reply to Julian Maurice from comment #21)
> > > Also, since it's a developer-only tool, do we need to restrict ourselves to
> > > an old version ?
> > 
> > We could package the latest version on CPAN and have it in our repo.
> > 
> > Mason, would that work for you?
> 
> sure, i've added the latest perltidy (20230309) to the dev/unstable/master
> suite in koha-staging repo 
> 
> if it tests ok, ill push it everywhere
> 
> 
> # apt policy perltidy
> perltidy:
>   Installed: 20230309-1~koha1
>   Candidate: 20230309-1~koha1
>   Version table:
>  *** 20230309-1~koha1 500
>         500 http://debian.koha-community.org/koha-staging dev/main amd64
> Packages
>         100 /var/lib/dpkg/status
>      20220613-1 500
>         500 http://ftp.nz.debian.org/debian sid/main amd64 Packages

Thanks, Mason! Yes, please push it!

Created attachment 152263 [details] [review]
Bug 30002: (QA follow-up) Add .perltidyrc to Makefile.PL mapping

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

> Thanks, Mason! Yes, please push it!

ok, pushed :)

I've updated the Perltidy page on the Wiki with some consistent structure and formatting - feel free to edit to fix up any errors that I may have inadvertently introduced:
https://wiki.koha-community.org/wiki/Perltidy

Questions:

Q1. I'm not sure what the perltidy -pro=.perltidyrc command is supposed to do - it did nothing for me in KTD (pertidy version is 20200110), it just sat there with a flashing cursor. Outside KTD with perltidy installed, it did the same as well.

Q2. For usage in Geany, I'm not sure what "Select part of your file or script to perltidy -pro=/home/path/to/.perltidyrc" means either.

(Possibly, the answer to these questions are obvious to developers 8-).)

(In reply to Jonathan Druart from comment #10)
> For instance I'd like to see
> 
> my $h = { one => 1, two => 3, };
> 
> always on 2 lines
> 
> my $h = {
>     one => 1,
>     two => 3,
> };

Yeah you like that, but I dont :)

It is not very clear what we exactly need to change now (or even why).
A simple example.

IIRC we needed to do this (no space after if and before parenthesis, but spaces around the condition):
if( $a ) {
But perltidy changes this to if ($a) {
Space after if, and no spaces around one condition.

But hey, things change for
if( $a && $b ) {
This should be now: if ( $a && $b ) {
Spaces around multiple conditions.

Similar thing for function calls:
Instead of test( 1 ) we now need test(1)
But test( 1, 2 ) is fine.

These are indeed stupid examples, but probably the things that we type most of the time.

We should imo not fail patches for occurrences of if( $a ) or test( 1 ), since that was our style until this report..

> We should imo not fail patches for occurrences of if( $a ) or test( 1 ), since that was our style until this report..

"our" style? We didn't have any styles :D


We simply need consistency.
The QA check will WARN instead of FAIL to tell QA that it's not a strict failure.

IMO we should fail (or adjust ourselves) for new blocks or new files. A couple of lines added to existing code should stick to the existing style.

We can add or adjust options to perltidyrc, and discuss them.

(In reply to Marcel de Rooy from comment #41)
> It is not very clear what we exactly need to change now (or even why).
> A simple example.
> 
> IIRC we needed to do this (no space after if and before parenthesis, but
> spaces around the condition):
> if( $a ) {
> But perltidy changes this to if ($a) {
> Space after if, and no spaces around one condition.
> 
> But hey, things change for
> if( $a && $b ) {
> This should be now: if ( $a && $b ) {
> Spaces around multiple conditions.
> 
> Similar thing for function calls:
> Instead of test( 1 ) we now need test(1)
> But test( 1, 2 ) is fine.

If these are the default perltidy's rules and there are no consensus in the team, we must pick the default perltidy's behaviour.

(In reply to Jonathan Druart from comment #43)

> If these are the default perltidy's rules and there are no consensus in the
> team, we must pick the default perltidy's behaviour.

Agreed

(In reply to David Nind from comment #39)
> I've updated the Perltidy page on the Wiki with some consistent structure
> and formatting - feel free to edit to fix up any errors that I may have
> inadvertently introduced:
> https://wiki.koha-community.org/wiki/Perltidy
> 
> Questions:
> 
> Q1. I'm not sure what the perltidy -pro=.perltidyrc command is supposed to
> do - it did nothing for me in KTD (pertidy version is 20200110), it just sat
> there with a flashing cursor. Outside KTD with perltidy installed, it did
> the same as well.
> 
> Q2. For usage in Geany, I'm not sure what "Select part of your file or
> script to perltidy -pro=/home/path/to/.perltidyrc" means either.
> 
> (Possibly, the answer to these questions are obvious to developers 8-).)

--pro= takes the path to Koha's perltidyrc file. I have updated the section for Geany again.

(In reply to Katrin Fischer from comment #45)

> --pro= takes the path to Koha's perltidyrc file. I have updated the section
> for Geany again.

Thanks Katrin!

Thanks for all the hard work!

Pushed to 23.05.x for the next release

Nice work everyone!

Pushed to oldstable for 22.11.x

(In reply to Mason James from comment #38)
> > Thanks, Mason! Yes, please push it!
> 
> ok, pushed :)

We have 20230309-1~koha1 but it's not the version install in ktd.

root@kohadevbox:koha$ perltidy --version
This is perltidy, v20200110

(In reply to Jonathan Druart from comment #49)
> (In reply to Mason James from comment #38)
> > > Thanks, Mason! Yes, please push it!
> > 
> > ok, pushed :)
> 
> We have 20230309-1~koha1 but it's not the version install in ktd.
> 
> root@kohadevbox:koha$ perltidy --version
> This is perltidy, v20200110

Ping Mason?

Mason, to clarify: we need 20230309-1~koha3 installed by default in ktd.