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.
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
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
Created attachment 177972 [details] [review] Bug 39115: Remove exclude list from tt_tidy.t
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.
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>
Created attachment 177980 [details] [review] Bug 39115: Remove exclude list from tt_tidy.t Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
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
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.
Created attachment 177984 [details] [review] Bug 39115: Remove exclude list from js_tidy.t
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!
(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
Created attachment 178003 [details] [review] Bug 39115: Restore ability to tidy files not in git index (yet)
(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.
(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
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
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
> 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.
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>
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>
Created attachment 178050 [details] [review] Bug 39115: Remove exclude list from js_tidy.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
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>
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>
# 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
(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
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>
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>
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>
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>
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>
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>
Makes sense. No QA script failures. Tests pass with the latest KTD as noted by Joubu.
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.
> 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,
(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
It works for the 3 file type. Thanks a lot :D Back to passed QA.
Created attachment 178127 [details] [review] Bug 39115: (follow-up) Exclude Schema.pm from perltidy.t
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.
Pushed for 25.05! Well done everyone, thank you!