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
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?
(In reply to Jonathan Druart from comment #50) > (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? Should we not fix ktd to use the newer version? A lot of people might already be using the newer version.
Katrin and I do not have the same version installed. I pulled this morning, ktd (KOHA_IMAGE=master) 12:18 < cait> perltidy: 12:18 < cait> Installed: 20230309-1~koha3 12:18 < cait> Candidate: 20230309-1~koha3 12:18 < cait> Version table: 12:18 < cait> *** 20230309-1~koha3 500 12:18 < cait> 500 http://debian.koha-community.org/koha-staging dev/main amd64 Packages 12:18 < cait> 100 /var/lib/dpkg/status 12:18 < cait> 20200110-1 500 12:18 < cait> 500 https://ftp.uni-stuttgart.de/debian bullseye/main amd64 Packages 12:19 < Joubu> I don't have 2023 installed 12:19 -!- Irssi: Pasting 5 lines to #koha. Press Ctrl-K if you wish to do this or Ctrl-C to cancel. 12:19 < Joubu> Version table: 12:19 < Joubu> 20230309-1~koha3 500 12:19 < Joubu> 500 http://debian.koha-community.org/koha-staging dev/main amd64 Packages 12:19 < Joubu> *** 20200110-1 500 12:19 < Joubu> 500 http://deb.debian.org/debian bullseye/main amd64 Packages 12:19 < Joubu> 100 /var/lib/dpkg/status Mason, how is that possible?
Mason, to clarify: we need 20230309-1~koha3 installed by default in ktd.