Bug 39115 - Tidy script should list the files we do not want to tidy
Summary: Tidy script should list the files we do not want to tidy
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi (tcohen)
URL:
Keywords: RM_priority
Depends on: 38664 39119
Blocks: 39130 39136
  Show dependency treegraph
 
Reported: 2025-02-12 19:57 UTC by Jonathan Druart
Modified: 2025-02-17 12:11 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 39115: Do not tidy files that must not be touched (5.71 KB, patch)
2025-02-12 20:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 39115: Remove exclude list from tt_tidy.t (751 bytes, patch)
2025-02-12 20:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 39115: Do not tidy files that must not be touched (5.77 KB, patch)
2025-02-13 04:59 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 39115: Remove exclude list from tt_tidy.t (807 bytes, patch)
2025-02-13 04:59 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 39115: Remove exclude list from js_tidy.t (822 bytes, patch)
2025-02-13 08:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 39115: Replace (:exclude) with --exclude (1.24 KB, patch)
2025-02-13 08:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 39115: Restore ability to tidy files not in git index (yet) (1.78 KB, patch)
2025-02-13 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 39115: Finally use ':(exclude)' and grep (1.40 KB, patch)
2025-02-13 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 39115: Do not tidy files that must not be touched (5.82 KB, patch)
2025-02-13 19:46 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 39115: Remove exclude list from tt_tidy.t (865 bytes, patch)
2025-02-13 19:46 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 39115: Remove exclude list from js_tidy.t (878 bytes, patch)
2025-02-13 19:46 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 39115: Replace (:exclude) with --exclude (1.29 KB, patch)
2025-02-13 19:47 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 39115: Restore ability to tidy files not in git index (yet) (1.83 KB, patch)
2025-02-13 19:47 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 39115: Finally use ':(exclude)' and grep (1.45 KB, patch)
2025-02-13 19:47 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 39115: Do not tidy files that must not be touched (5.88 KB, patch)
2025-02-14 17:24 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39115: Remove exclude list from tt_tidy.t (920 bytes, patch)
2025-02-14 17:24 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39115: Remove exclude list from js_tidy.t (933 bytes, patch)
2025-02-14 17:24 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39115: Replace (:exclude) with --exclude (1.35 KB, patch)
2025-02-14 17:24 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39115: Restore ability to tidy files not in git index (yet) (1.88 KB, patch)
2025-02-14 17:24 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39115: Finally use ':(exclude)' and grep (1.51 KB, patch)
2025-02-14 17:25 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39115: (follow-up) Exclude Schema.pm from perltidy.t (919 bytes, patch)
2025-02-16 05:20 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2025-02-12 19:57:36 UTC
It should prevent to tidy files that must be kept unchanged.

It was initially a feature (the script does what you ask it to do), but it's a mess maintain the list of files we want to keep tidy.
Comment 1 Victor Grousset/tuxayo 2025-02-12 20:05:10 UTC
The 4 places that have/need the denylist

misc/devel/tidy.pl
push @files, qx{git ls-files '*.pl' '*.pm' '*.t' ':(exclude)Koha/Schema/Result' ':(exclude)Koha/Schema.pm'};


xt/perltidy.t
push @files, qx{git ls-files '*.pl' '*.pm' '*.t' ':(exclude)Koha/Schema/Result' ':(exclude)Koha/Schema.pm'};


KTD: pre-commit hook: https://gitlab.com/koha-community/koha-testing-docker/-/issues/478


QA tools: tidy check: https://gitlab.com/koha-community/qa-test-tools/-/issues/91
Comment 2 Jonathan Druart 2025-02-12 20:36:13 UTC
Created attachment 177971 [details] [review]
Bug 39115: Do not tidy files that must not be touched

It should prevent to tidy files that must be kept unchanged.

It was initially a feature (the script does what you ask it to do),
but it's a mess maintain the list of files we want to keep tidy.

Test plan:
Koha/Schema.pm is one of the file we don't want to modify/tidy
So let's play with this one, feel free to pick others (see the
exception list per filetype in the script)

 % perl misc/devel/tidy.pl --perl
will not touch Koha/Schema.pm

 % perl misc/devel/tidy.pl mainpage.pl Koha/Schema.pm
will not touch Koha/Schema.pm but mainpage.pl will

 % perl misc/devel/tidy.pl --no-write Koha/Schema.pm
The --no-write option is called by the QA script to compare the output
and see if files are tidy. We want to return something in this case. So
the file will be printed to STDOUT untouched
Comment 3 Jonathan Druart 2025-02-12 20:48:20 UTC
Created attachment 177972 [details] [review]
Bug 39115: Remove exclude list from tt_tidy.t
Comment 4 Victor Grousset/tuxayo 2025-02-12 21:47:14 UTC
WIP testing results:
I added some untidy stuff in Koha/Schema.pm and with the patches it's preserved :D

Output of the script:
2955 files on main
2952 files with the patches

Diffing the output gives this files that are not here anymore
Koha/Schema.pm: good
opac/svc/shelfbrowser.pl not being listed twice
svc/cataloguing/automatic_linker.pl not being listed twice

Weird but ok. I'll update with the rest of the testing.
Comment 5 Victor Grousset/tuxayo 2025-02-13 04:59:18 UTC
Created attachment 177979 [details] [review]
Bug 39115: Do not tidy files that must not be touched

It should prevent to tidy files that must be kept unchanged.

It was initially a feature (the script does what you ask it to do),
but it's a mess maintain the list of files we want to keep tidy.

Test plan:
Koha/Schema.pm is one of the file we don't want to modify/tidy
So let's play with this one, feel free to pick others (see the
exception list per filetype in the script)

 % perl misc/devel/tidy.pl --perl
will not touch Koha/Schema.pm

 % perl misc/devel/tidy.pl mainpage.pl Koha/Schema.pm
will not touch Koha/Schema.pm but mainpage.pl will

 % perl misc/devel/tidy.pl --no-write Koha/Schema.pm
The --no-write option is called by the QA script to compare the output
and see if files are tidy. We want to return something in this case. So
the file will be printed to STDOUT untouched

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 6 Victor Grousset/tuxayo 2025-02-13 04:59:20 UTC
Created attachment 177980 [details] [review]
Bug 39115: Remove exclude list from tt_tidy.t

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 7 Victor Grousset/tuxayo 2025-02-13 04:59:39 UTC
Rest of the test plan: ok

Confirmed it fixes directly: #478 QA tools: pre-commit hook shouldn't check tidyness on DBIX schema files
https://gitlab.com/koha-community/koha-testing-docker/-/issues/478

Confirmed it fixes directly: #91 KTD: tidyness check shouldn't happen on DBIX schema files
https://gitlab.com/koha-community/qa-test-tools/-/issues/91


testing notes: have a KTD repo up to date (for hooks)
And when having started KTD:
cd ../qa-test-tools/ ; sudo git pull  # to update the QA tools
Comment 8 Victor Grousset/tuxayo 2025-02-13 05:54:10 UTC
There is this separate occurrence of the perl denylist that is now diverging from tidy.pl:
xt/perltidy.t
7:push @files, qx{git ls-files '*.pl' '*.pm' '*.t' ':(exclude)Koha/Schema/Result'};
(missing Koha/Schema.pm)


js_tidy.t also has duplication of the list:
    qx{git ls-files '*.js' '*.ts' ':(exclude)koha-tmpl/intranet-tmpl/lib' ':(exclude)koha-tmpl/intranet-tmpl/js/Gettext.js' ':(exclude)koha-tmpl/opac-tmpl/lib' ':(exclude)Koha/ILL/Backend/'};


tt_tidy.t is good because it calls tidy.pl. That's why you could remove the denylist from there.
Comment 9 Jonathan Druart 2025-02-13 08:22:00 UTC
Created attachment 177984 [details] [review]
Bug 39115: Remove exclude list from js_tidy.t
Comment 10 Jonathan Druart 2025-02-13 08:39:20 UTC
Created attachment 177985 [details] [review]
Bug 39115: Replace (:exclude) with --exclude

I don't really explain this one, but this is something wrong with
(:exclude)

% git ls-files koha-tmpl/intranet-tmpl/js/i18n.js --exclude='koha-tmpl/intranet-tmpl/lib' --exclude='koha-tmpl/intranet-tmpl/js/Gettext.js' --exclude='koha-tmpl/opac-tmpl/lib'
koha-tmpl/intranet-tmpl/js/i18n.js
=> Return the file has expected

% git ls-files koha-tmpl/intranet-tmpl/js/i18n.js ':(exclude)koha-tmpl/intranet-tmpl/lib' ':(exclude)koha-tmpl/intranet-tmpl/js/Gettext.js' ':(exclude)koha-tmpl/opac-tmpl/lib'
=> Does not return anything!
Comment 11 Jonathan Druart 2025-02-13 08:41:10 UTC
(In reply to Jonathan Druart from comment #10)
> Created attachment 177985 [details] [review] [review]
> Bug 39115: Replace (:exclude) with --exclude
> 
> I don't really explain this one, but this is something wrong with
> (:exclude)
> 
> % git ls-files koha-tmpl/intranet-tmpl/js/i18n.js
> --exclude='koha-tmpl/intranet-tmpl/lib'
> --exclude='koha-tmpl/intranet-tmpl/js/Gettext.js'
> --exclude='koha-tmpl/opac-tmpl/lib'
> koha-tmpl/intranet-tmpl/js/i18n.js
> => Return the file has expected
> 
> % git ls-files koha-tmpl/intranet-tmpl/js/i18n.js
> ':(exclude)koha-tmpl/intranet-tmpl/lib'
> ':(exclude)koha-tmpl/intranet-tmpl/js/Gettext.js'
> ':(exclude)koha-tmpl/opac-tmpl/lib'
> => Does not return anything!

It seems that
 * --exclude filters after resolving the explicit file path
 * :(exclude) filters before resolving the file path, so even explicitly listed files can be excluded
Comment 12 Jonathan Druart 2025-02-13 13:06:37 UTC
Created attachment 178003 [details] [review]
Bug 39115: Restore ability to tidy files not in git index (yet)
Comment 13 Jonathan Druart 2025-02-13 13:07:52 UTC
(In reply to Jonathan Druart from comment #12)
> Created attachment 178003 [details] [review] [review]
> Bug 39115: Restore ability to tidy files not in git index (yet)

For instance:

perl misc/devel/tidy.pl --no-write koha-tmpl/intranet-tmpl/prog/fr-FR/modules/intranet-main.tt

perl misc/devel/tidy.pl koha-tmpl/intranet-tmpl/prog/fr-FR/modules/intranet-main.tt

perl misc/devel/tidy.pl koha-tmpl/intranet-tmpl/prog/fr-FR/modules/intranet-main.tt  koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt

Compare the result with and without the patch. Confirm that the new result are the expected one.
Comment 14 Jonathan Druart 2025-02-13 15:01:00 UTC
(In reply to Jonathan Druart from comment #13)
> (In reply to Jonathan Druart from comment #12)
> > Created attachment 178003 [details] [review] [review] [review]
> > Bug 39115: Restore ability to tidy files not in git index (yet)
> 
> For instance:
> 
> perl misc/devel/tidy.pl --no-write
> koha-tmpl/intranet-tmpl/prog/fr-FR/modules/intranet-main.tt
> 
> perl misc/devel/tidy.pl
> koha-tmpl/intranet-tmpl/prog/fr-FR/modules/intranet-main.tt
> 
> perl misc/devel/tidy.pl
> koha-tmpl/intranet-tmpl/prog/fr-FR/modules/intranet-main.tt 
> koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt
> 
> Compare the result with and without the patch. Confirm that the new result
> are the expected one.

But this is not working

% git ls-files Koha/Schema.pm --exclude='Koha/Schema/Result' --exclude='Koha/Schema.pm'
Koha/Schema.pm
Comment 15 Jonathan Druart 2025-02-13 15:05:35 UTC
Created attachment 178017 [details] [review]
Bug 39115: Finally use ':(exclude)' and grep

This was not working:
% git ls-files Koha/Schema.pm --exclude='Koha/Schema/Result' --exclude='Koha/Schema.pm'
Koha/Schema.pm

The efficiency of the script is getting worse and it will need to be
rewritten.
The xt/tt_tidy.t will take longer.

But at least it should work correctly now
Comment 16 Jonathan Druart 2025-02-13 15:09:25 UTC
You will need this patch to the pre-commit hook if you want to test the commit.

https://gitlab.com/koha-community/koha-testing-docker/-/commit/9b82c2d5417c4cf371e683b3760a6aa16b0d4b9a
Comment 17 Victor Grousset/tuxayo 2025-02-13 19:36:41 UTC
> The efficiency of the script is getting worse and it will need to be
rewritten.
> The xt/tt_tidy.t will take longer.

With and without the patches I get around 1110 seconds of time needed to run this locally.

Unless there is something else to do to see the issue, there is no worry about these patches making the CI time even longer.

----

But yesterday, it was taking 600 secs to run this. So another fresh patch might have slowed the test. I'll also dig into this after this ticket.
Comment 18 Nick Clemens (kidclamp) 2025-02-13 19:46:55 UTC
Created attachment 178048 [details] [review]
Bug 39115: Do not tidy files that must not be touched

It should prevent to tidy files that must be kept unchanged.

It was initially a feature (the script does what you ask it to do),
but it's a mess maintain the list of files we want to keep tidy.

Test plan:
Koha/Schema.pm is one of the file we don't want to modify/tidy
So let's play with this one, feel free to pick others (see the
exception list per filetype in the script)

 % perl misc/devel/tidy.pl --perl
will not touch Koha/Schema.pm

 % perl misc/devel/tidy.pl mainpage.pl Koha/Schema.pm
will not touch Koha/Schema.pm but mainpage.pl will

 % perl misc/devel/tidy.pl --no-write Koha/Schema.pm
The --no-write option is called by the QA script to compare the output
and see if files are tidy. We want to return something in this case. So
the file will be printed to STDOUT untouched

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Nick Clemens (kidclamp) 2025-02-13 19:46:57 UTC
Created attachment 178049 [details] [review]
Bug 39115: Remove exclude list from tt_tidy.t

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 20 Nick Clemens (kidclamp) 2025-02-13 19:46:59 UTC
Created attachment 178050 [details] [review]
Bug 39115: Remove exclude list from js_tidy.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 21 Nick Clemens (kidclamp) 2025-02-13 19:47:01 UTC
Created attachment 178051 [details] [review]
Bug 39115: Replace (:exclude) with --exclude

I don't really explain this one, but this is something wrong with
(:exclude)

% git ls-files koha-tmpl/intranet-tmpl/js/i18n.js --exclude='koha-tmpl/intranet-tmpl/lib' --exclude='koha-tmpl/intranet-tmpl/js/Gettext.js' --exclude='koha-tmpl/opac-tmpl/lib'
koha-tmpl/intranet-tmpl/js/i18n.js
=> Return the file has expected

% git ls-files koha-tmpl/intranet-tmpl/js/i18n.js ':(exclude)koha-tmpl/intranet-tmpl/lib' ':(exclude)koha-tmpl/intranet-tmpl/js/Gettext.js' ':(exclude)koha-tmpl/opac-tmpl/lib'
=> Does not return anything!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 22 Nick Clemens (kidclamp) 2025-02-13 19:47:03 UTC
Created attachment 178052 [details] [review]
Bug 39115: Restore ability to tidy files not in git index (yet)

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Nick Clemens (kidclamp) 2025-02-13 19:47:05 UTC
Created attachment 178053 [details] [review]
Bug 39115: Finally use ':(exclude)' and grep

This was not working:
% git ls-files Koha/Schema.pm --exclude='Koha/Schema/Result' --exclude='Koha/Schema.pm'
Koha/Schema.pm

The efficiency of the script is getting worse and it will need to be
rewritten.
The xt/tt_tidy.t will take longer.

But at least it should work correctly now

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 24 Nick Clemens (kidclamp) 2025-02-13 19:47:56 UTC
#   Failed test 'koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/tables/shelves_results.tt should be kept tidy'
#   at /kohadevbox/koha/xt/tt_tidy.t line 46.

#   Failed test 't/db_dependent/misc/translator/sample.tt should be kept tidy'
#   at /kohadevbox/koha/xt/tt_tidy.t line 46.
# Looks like you failed 2 tests of 828.
Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/828 subtests 

Test Summary Report
-------------------
/kohadevbox/koha/xt/tt_tidy.t (Wstat: 512 (exited 2) Tests: 828 Failed: 2)
  Failed tests:  679, 822
  Non-zero exit status: 2
Files=1, Tests=828, 137 wallclock secs ( 0.05 usr  0.01 sys + 1594.13 cusr 306.97 csys = 1901.16 CPU)
Result: FAIL

I got the same failure with and without these patches, all else worked great, applied and tested hook
Comment 25 Jonathan Druart 2025-02-14 12:28:46 UTC
(In reply to Nick Clemens (kidclamp) from comment #24)
> #   Failed test
> 'koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/tables/
> shelves_results.tt should be kept tidy'
> #   at /kohadevbox/koha/xt/tt_tidy.t line 46.
> 
> #   Failed test 't/db_dependent/misc/translator/sample.tt should be kept
> tidy'
> #   at /kohadevbox/koha/xt/tt_tidy.t line 46.
> # Looks like you failed 2 tests of 828.
> Dubious, test returned 2 (wstat 512, 0x200)
> Failed 2/828 subtests 
> 
> Test Summary Report
> -------------------
> /kohadevbox/koha/xt/tt_tidy.t (Wstat: 512 (exited 2) Tests: 828 Failed: 2)
>   Failed tests:  679, 822
>   Non-zero exit status: 2
> Files=1, Tests=828, 137 wallclock secs ( 0.05 usr  0.01 sys + 1594.13 cusr
> 306.97 csys = 1901.16 CPU)
> Result: FAIL
> 
> I got the same failure with and without these patches, all else worked
> great, applied and tested hook

My guess it that you don't have the new version of the prettier plugin, try `yarn install` or remove your local node_modules and pull the new ktd images.

https://gitlab.com/koha-community/prettier-plugin-template-toolkit/-/commits/main?ref_type=heads
You need 2.0.2
Comment 26 Tomás Cohen Arazi (tcohen) 2025-02-14 17:24:48 UTC
Created attachment 178108 [details] [review]
Bug 39115: Do not tidy files that must not be touched

It should prevent to tidy files that must be kept unchanged.

It was initially a feature (the script does what you ask it to do),
but it's a mess maintain the list of files we want to keep tidy.

Test plan:
Koha/Schema.pm is one of the file we don't want to modify/tidy
So let's play with this one, feel free to pick others (see the
exception list per filetype in the script)

 % perl misc/devel/tidy.pl --perl
will not touch Koha/Schema.pm

 % perl misc/devel/tidy.pl mainpage.pl Koha/Schema.pm
will not touch Koha/Schema.pm but mainpage.pl will

 % perl misc/devel/tidy.pl --no-write Koha/Schema.pm
The --no-write option is called by the QA script to compare the output
and see if files are tidy. We want to return something in this case. So
the file will be printed to STDOUT untouched

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi (tcohen) 2025-02-14 17:24:50 UTC
Created attachment 178109 [details] [review]
Bug 39115: Remove exclude list from tt_tidy.t

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi (tcohen) 2025-02-14 17:24:53 UTC
Created attachment 178110 [details] [review]
Bug 39115: Remove exclude list from js_tidy.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Tomás Cohen Arazi (tcohen) 2025-02-14 17:24:55 UTC
Created attachment 178111 [details] [review]
Bug 39115: Replace (:exclude) with --exclude

I don't really explain this one, but this is something wrong with
(:exclude)

% git ls-files koha-tmpl/intranet-tmpl/js/i18n.js --exclude='koha-tmpl/intranet-tmpl/lib' --exclude='koha-tmpl/intranet-tmpl/js/Gettext.js' --exclude='koha-tmpl/opac-tmpl/lib'
koha-tmpl/intranet-tmpl/js/i18n.js
=> Return the file has expected

% git ls-files koha-tmpl/intranet-tmpl/js/i18n.js ':(exclude)koha-tmpl/intranet-tmpl/lib' ':(exclude)koha-tmpl/intranet-tmpl/js/Gettext.js' ':(exclude)koha-tmpl/opac-tmpl/lib'
=> Does not return anything!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Tomás Cohen Arazi (tcohen) 2025-02-14 17:24:57 UTC
Created attachment 178112 [details] [review]
Bug 39115: Restore ability to tidy files not in git index (yet)

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 31 Tomás Cohen Arazi (tcohen) 2025-02-14 17:25:00 UTC
Created attachment 178113 [details] [review]
Bug 39115: Finally use ':(exclude)' and grep

This was not working:
% git ls-files Koha/Schema.pm --exclude='Koha/Schema/Result' --exclude='Koha/Schema.pm'
Koha/Schema.pm

The efficiency of the script is getting worse and it will need to be
rewritten.
The xt/tt_tidy.t will take longer.

But at least it should work correctly now

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 32 Tomás Cohen Arazi (tcohen) 2025-02-14 17:25:35 UTC
Makes sense. No QA script failures. Tests pass with the latest KTD as noted by Joubu.
Comment 33 Victor Grousset/tuxayo 2025-02-15 01:10:59 UTC
Additional testing with these files to confirm the pre-commit hook and the QA script ignores them as expected:

Koha/Schema.pm
Koha/Schema/Result/City.pm
koha-tmpl/intranet-tmpl/lib/greybox/AJS.js
koha-tmpl/intranet-tmpl/prog/en/includes/doc-head-open.inc

For other files, the hook tidies them as expected.
The script QA doesn't complain but that will be a ticket I'll open for the QA tools.

The perl part of the denylist is still duplicated with another instance still in xt/perltidy.t
That will be for a follow-up ticket I'll open. To either remove that last redundancy. Or have a comment mentioning it to keep both in sync.
Comment 34 Victor Grousset/tuxayo 2025-02-15 01:24:05 UTC
> The script QA doesn't complain but that will be a ticket I'll open for the QA tools.

Oh no, actually the QA script not complaining about legit tidy issues is partially from these patches!

--on main:------------------------------
 FAIL   koha-tmpl/intranet-tmpl/js/i18n.js
   FAIL   tidiness
                File is not tidy, please run `perl misc/devel/tidy.pl /kohadevbox/koha/koha-tmpl/intranet-tmpl/js/i18n.js`

 OK     koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt

 FAIL   mainpage.pl
   FAIL   tidiness
                File is not tidy, please run `perl misc/devel/tidy.pl /kohadevbox/koha/mainpage.pl`
-----------------------

It was already missing intranet-main.tt. But it was working for the .pl and .js files.
So that's a regression these patches will be introducing :/


------------------------

Testing details in case I missed something:


----QA tools is on latest commit in my KTD---------------------------------
commit 11502a72aa1c77f9c981299a81a6d434e54e2808
Issue #90: Fix prettierrc.js filename




-----name and diff of the files I untidied------------------------

diff --git a/koha-tmpl/intranet-tmpl/js/i18n.js b/koha-tmpl/intranet-tmpl/js/i18n.js
index 6eb7e86a8d..d6a7ef3f5b 100644
--- a/koha-tmpl/intranet-tmpl/js/i18n.js
+++ b/koha-tmpl/intranet-tmpl/js/i18n.js
@@ -46,6 +46,6 @@ function __np(msgctxt, msgid, msgid_plural, count) {
     return Koha.i18n.gt.npgettext(msgctxt, msgid, msgid_plural, count);
 }

-function __npx(msgctxt, msgid, msgid_plural, count, vars) {
+         function __npx(msgctxt, msgid, msgid_plural, count, vars) {
     return Koha.i18n.expand(__np(msgctxt, msgid, msgid_plural, count), vars);
 }
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt
index 91172dac2a..69faa7ff92 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt
@@ -340,6 +340,6 @@
             });
         });
     </script>
-[% END %]
+      [% END %]
 <!-- the main div is closed in intranet-bottom.inc -->
 [% INCLUDE 'intranet-bottom.inc' %]
diff --git a/mainpage.pl b/mainpage.pl
index 87f784631f..1d948089aa 100755
--- a/mainpage.pl
+++ b/mainpage.pl
@@ -153,7 +153,7 @@ if ( C4::Context->preference('PatronSelfRegistrationAlert') ) {
     );
 }

-$template->param(
+            $template->param(
     pendingcomments                => $pendingcomments,
     pendingtags                    => $pendingtags,
     pending_borrower_modifications => $pending_borrower_modifications,
Comment 35 Jonathan Druart 2025-02-15 04:57:10 UTC
(In reply to Victor Grousset/tuxayo from comment #34)
> > The script QA doesn't complain but that will be a ticket I'll open for the QA tools.

I actually fixed that yesterday but forgot to push it, and it was only inside ktd as I didn't find the commit on my host.

It's caused by the grep -v and the use of abspath by the QA script.

See https://gitlab.com/koha-community/qa-test-tools/-/commit/2c62b862ef264d9fb427111cedb8b0e9673f8ca1
Comment 36 Victor Grousset/tuxayo 2025-02-15 13:05:54 UTC
It works for the 3 file type. Thanks a lot :D
Back to passed QA.
Comment 37 Victor Grousset/tuxayo 2025-02-16 05:20:47 UTC
Created attachment 178127 [details] [review]
Bug 39115: (follow-up) Exclude Schema.pm from perltidy.t
Comment 38 Victor Grousset/tuxayo 2025-02-16 05:24:57 UTC
Follow-up to fix perltidy.t.
In Bug 39132 comment 2, that change was planned to be included here.
Maybe the plan was to eliminate the last duplication of the denylist, but at this point, that would be for a follow-up. Let's fix the tests first by fixing the 2nd denylist.
Comment 39 Katrin Fischer 2025-02-17 12:11:00 UTC
Pushed for 25.05!

Well done everyone, thank you!