Bug 30002 - Add project-level perltidyrc
Summary: Add project-level perltidyrc
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords: rel_23_11_candidate
Depends on:
Blocks:
 
Reported: 2022-02-02 06:09 UTC by Victor Grousset/tuxayo
Modified: 2023-12-19 11:27 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.02,22.11.08


Attachments
Bug 30002: Add .perltidyrc (982 bytes, patch)
2023-03-31 10:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 30002: Remove unused xt/perltidyrc (2.44 KB, patch)
2023-03-31 10:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 30002: Adjust perltidy (1.16 KB, patch)
2023-05-02 12:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30002: Add .perltidyrc (1.02 KB, patch)
2023-06-08 11:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30002: Remove unused xt/perltidyrc (2.49 KB, patch)
2023-06-08 11:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30002: Adjust perltidy (1.22 KB, patch)
2023-06-08 11:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30002: (QA follow-up) Add .perltidyrc to Makefile.PL mapping (879 bytes, patch)
2023-06-09 18:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Grousset/tuxayo 2022-02-02 06:09:59 UTC
It doesn't influence the tests at all.
After emptying it or removing or setting the max line length to 40 char no error pops.

So it was proposed for removal in last dev meeting:
http://irc.koha-community.org/koha/2022-01-19#i_2400452
Comment 1 Jonathan Druart 2022-12-14 11:03:58 UTC
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.
Comment 2 Julian Maurice 2023-03-30 13:20:50 UTC
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 ?
Comment 3 Julian Maurice 2023-03-31 10:50:47 UTC
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.
Comment 4 Julian Maurice 2023-03-31 10:50:49 UTC
Created attachment 149028 [details] [review]
Bug 30002: Remove unused xt/perltidyrc
Comment 5 Julian Maurice 2023-03-31 11:57:46 UTC
(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
Comment 6 Jonathan Druart 2023-03-31 12:37:26 UTC
I would extend the max line length

--maximum-line-length=178
Comment 7 Julian Maurice 2023-03-31 14:24:30 UTC
(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.
Comment 8 Tomás Cohen Arazi 2023-04-17 11:40:20 UTC
What is the status of this?
Comment 9 Julian Maurice 2023-04-17 12:03:18 UTC
This need to be reviewed by QA team, so... Signed Off ?
Comment 10 Jonathan Druart 2023-04-17 12:15:28 UTC
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.
Comment 11 Julian Maurice 2023-04-17 12:25:44 UTC
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.
Comment 12 Julian Maurice 2023-04-17 12:27:46 UTC
--comma-arrow-breakpoints=0 appears to solve the issue as well.
Comment 13 Martin Renvoize 2023-04-17 13:52:41 UTC
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.
Comment 14 Katrin Fischer 2023-04-23 09:51:21 UTC
How to move this forward the best?

I think we need to decide on line length, does 120 sound like a reasonable compromise?
Comment 15 Jonathan Druart 2023-04-25 12:37:30 UTC
I will be on it, I just need more time...
Comment 16 David Nind 2023-04-29 21:53:08 UTC
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
Comment 17 Jonathan Druart 2023-05-02 07:00:14 UTC
(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.
Comment 18 Jonathan Druart 2023-05-02 12:43:52 UTC
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
Comment 19 Jonathan Druart 2023-05-02 12:46:36 UTC
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.
Comment 20 Jonathan Druart 2023-05-02 12:48:00 UTC
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!
Comment 21 Julian Maurice 2023-05-02 13:05:12 UTC
(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)
Comment 22 Jonathan Druart 2023-05-02 13:40:06 UTC
(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!
Comment 23 Jonathan Druart 2023-06-01 07:59:11 UTC
(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?
Comment 24 Jonathan Druart 2023-06-01 08:56:56 UTC
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.
"""
Comment 25 Jonathan Druart 2023-06-01 08:57:39 UTC
Please consider this as a candidate for 23.11 early pushes.
Comment 26 Tomás Cohen Arazi 2023-06-08 11:25:14 UTC
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>
Comment 27 Tomás Cohen Arazi 2023-06-08 11:25:17 UTC
Created attachment 152162 [details] [review]
Bug 30002: Remove unused xt/perltidyrc

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2023-06-08 11:25:21 UTC
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>
Comment 29 Tomás Cohen Arazi 2023-06-08 11:33:21 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 30 Jonathan Druart 2023-06-08 13:38:28 UTC
(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.
Comment 31 Tomás Cohen Arazi 2023-06-08 19:13:45 UTC
(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.
Comment 32 Katrin Fischer 2023-06-08 19:30:09 UTC
(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.
Comment 33 David Cook 2023-06-09 00:10:13 UTC
Does anything need to be done for folk who code in koha-testing-docker using vim?
Comment 34 Mason James 2023-06-09 03:45:03 UTC
(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
Comment 35 Jonathan Druart 2023-06-09 07:00:44 UTC
(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?
Comment 36 Jonathan Druart 2023-06-09 07:01:21 UTC
(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!
Comment 37 Tomás Cohen Arazi 2023-06-09 18:42:51 UTC
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>
Comment 38 Mason James 2023-06-10 22:59:09 UTC
> Thanks, Mason! Yes, please push it!

ok, pushed :)
Comment 39 David Nind 2023-06-14 09:11:29 UTC
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-).)
Comment 40 Marcel de Rooy 2023-06-15 12:32:11 UTC
(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 :)
Comment 41 Marcel de Rooy 2023-06-15 12:41:32 UTC
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..
Comment 42 Jonathan Druart 2023-06-16 06:59:32 UTC
> 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.
Comment 43 Jonathan Druart 2023-06-16 07:01:22 UTC
(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.
Comment 44 Marcel de Rooy 2023-06-16 07:04:05 UTC
(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
Comment 45 Katrin Fischer 2023-06-16 19:06:06 UTC
(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.
Comment 46 David Nind 2023-06-16 22:01:36 UTC
(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!
Comment 47 Martin Renvoize 2023-07-12 07:01:33 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 48 Matt Blenkinsop 2023-07-12 09:16:52 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x
Comment 49 Jonathan Druart 2023-09-06 13:09:26 UTC
(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
Comment 50 Jonathan Druart 2023-12-19 11:01:53 UTC
(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?
Comment 51 Katrin Fischer 2023-12-19 11:05:37 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2023-12-19 11:25:11 UTC Comment hidden (obsolete)
Comment 53 Jonathan Druart 2023-12-19 11:27:02 UTC
Mason, to clarify: we need 20230309-1~koha3 installed by default in ktd.