We should tidy our codebase and ensure to keep it tidy. I am suggesting to have patches for 25.05 that will tidy our perl (pl, pm, t), javascript (js, ts, vue) and template (tt, inc) files. We will need: * a pre-commit git hook * a xt test (can be very slow however, needs to be discussed) * configure .git-blame-ignore-revs (see `git blame --ignore-revs-file .git-blame-ignore-revs) * provide a script to ease rebases
I can work on this as soon as we agree we want it for 25.05.
I want it!
I'm not against it, but I don't see this as being high priority either.
(In reply to Pedro Amorim from comment #3) > I'm not against it, but I don't see this as being high priority either. This is a priority for me because of the number of commits dealing with tidying: % git log --oneline v24.05.00..HEAD|grep -i tidy|wc -l 80 We lose time during QA.
This might be personal to me, but I think we also lose time in dev. Uniformly formatted code is easier to scan in my opinion. Also we don't need to format ranges anymore and can straight go to format on save.
With a good pre-commit hook we won't need to care about formatting/tidying anymore. Just code and commit (what we do already for .vue files).
(In reply to Jonathan Druart from comment #4) > This is a priority for me because of the number of commits dealing with > tidying: > > % git log --oneline v24.05.00..HEAD|grep -i tidy|wc -l > 80 > > We lose time during QA. These are tidy patches on top of submissions, tidying the whole code won't stop these. New submissions will potentially continue to be submitted with untidy code and tidy fixes will still continue to exist to address these. Squashing tidy fixes into the original patch is what will stop these. Correct me if I'm wrong please in case I'm missing the point. As I said, I'm not against it, I just don't see how tidying the whole codebase is fixing any problem. (In reply to Paul Derscheid from comment #5) > This might be personal to me, but I think we also lose time in dev. > Uniformly formatted code is easier to scan in my opinion. > > Also we don't need to format ranges anymore and can straight go to format on > save. I've been using format on paste. Cut whatever new code I wrote and paste -> its tidy. I'm okay being in the minority here, just giving my opinion as it was asked!
(In reply to Pedro Amorim from comment #7) > (In reply to Jonathan Druart from comment #4) > > This is a priority for me because of the number of commits dealing with > > tidying: > > > > % git log --oneline v24.05.00..HEAD|grep -i tidy|wc -l > > 80 > > > > We lose time during QA. > > These are tidy patches on top of submissions, tidying the whole code won't > stop these. New submissions will potentially continue to be submitted with > untidy code and tidy fixes will still continue to exist to address these. > Squashing tidy fixes into the original patch is what will stop these. > Correct me if I'm wrong please in case I'm missing the point. > > As I said, I'm not against it, I just don't see how tidying the whole > codebase is fixing any problem. See comment 6. With a pre-commit hook you don't need to care about tidying at all. Just code and commit. With everybody using the hook we ensure that the code stays tidy. For that we need a starting point where everything is tidy. Regular contributors won't submit code that is not tidy as they are using ktd with the git hook installed automatically. On top of that, add a QA check (if someone forgets to setup the hook) and a xt test (if QA+RM forgets to run the QA script), and the code stays tidy, forever :)
(In reply to Pedro Amorim from comment #3) > I'm not against it, but I don't see this as being high priority either. +1
I've started to come around to the idea more after participating in the Mattermost chat, but then I realised there is 1 potential problem with the pre-commit git hook. perltidy doesn't always work. Sometimes it fails and you end up with inline error messages. For a big dev, this could be a risk. I suppose --no-verify could bypass the auto-perltidy and you could always still provide your own Tidy follow-up if perltidy is failing? Just food for thought...
> On top of that, add a QA check (if someone forgets to setup the hook) and a > xt test (if QA+RM forgets to run the QA script), and the code stays tidy, > forever :) If it is only caught by the xt test, then it means the main branch is not tidy (I assume the test is run by Jenkins, not by RM). So perltidy will still need to be run from time to time ? Not a big deal for me, I prefer having a code base 100% tidy most of the time than the current state. Side note: are you considering using https://pre-commit.com/ ? I think it would be handy when dealing with multiple dev installs, and it would be easier to update/add new hooks if needed. > perltidy doesn't always work. Sometimes it fails and you end up with inline > error messages. I've seen it failed too, but only if given invalid/incomplete Perl code (a range of lines ending in the middle of an if-block for instance). Can it fail on valid Perl code ? Another advantage I see is that the QA script won't need to run perltidy test on the first pass, so it should speed things up a little bit. (off-topic: it would be great if we didn't need that 1st pass at all: no need to commit, and cleaner git reflog) > * provide a script to ease rebases You got me curious there... Do you have a magic spell or something ? :)
Just anecdotal evidence, I never had perltidy fail on valid code, ever. Grain of salt advised, though.
I am probably not the right person to assess the technical consequences, but I feel it's important we reach consensus on this as it will affect any outstanding patches. I would like to put the timing not before January because of that, because it would give us some time to process older code in the QA queue, while we can already start more rigorous tidying for new patches maybe? For info: this RM runs xt tests before pushing :)
(In reply to David Cook from comment #10) > I've started to come around to the idea more after participating in the > Mattermost chat, but then I realised there is 1 potential problem with the > pre-commit git hook. > > perltidy doesn't always work. Sometimes it fails and you end up with inline > error messages. For a big dev, this could be a risk. > > I suppose --no-verify could bypass the auto-perltidy and you could always > still provide your own Tidy follow-up if perltidy is failing? > > Just food for thought... Good point, the hook can prevent the commit if the tidy command fails and the files won't be modified (will need a copy before). (In reply to Julian Maurice from comment #11) > > On top of that, add a QA check (if someone forgets to setup the hook) and a > > xt test (if QA+RM forgets to run the QA script), and the code stays tidy, > > forever :) > If it is only caught by the xt test, then it means the main branch is not > tidy (I assume the test is run by Jenkins, not by RM). So perltidy will > still need to be run from time to time ? Not a big deal for me, I prefer > having a code base 100% tidy most of the time than the current state. If we have a pre-commit hook to prevent authors to commit non-tidy code and a QA check to prevent PQA or push of non-tidy code, it should be enough already. The xt test is yet another protection but it should not be needed (but we are humans :D). > Side note: are you considering using https://pre-commit.com/ ? I think it > would be handy when dealing with multiple dev installs, and it would be > easier to update/add new hooks if needed. I did when I introduce the first one, but it seems too much overhead for our simple use cases. > > * provide a script to ease rebases > You got me curious there... Do you have a magic spell or something ? :) The idea (not tested yet and needs to be confirmed/improved/discussed) is to: 0. have a patch set for bug 12345 that has been written before the tidy commit(s) 1. run "the rebase script" a. checkout commit before the tidy bug b. apply patches from 12345 one-by-one c. run the tidy script on each of the commit d. rebase and keep changes from 12345: git checkout --theirs (or git rebase -Xtheirs) 2. Attach the rebased version of the patches The script would then deal with everything for you, apart from the conflicts you would have gotten anyway. We will need to stop the "keep theirs" behaviour after we applied the tidy commits however. (If I have not mixed up ours and theirs once again) (In reply to Katrin Fischer from comment #13) > I am probably not the right person to assess the technical consequences, but > I feel it's important we reach consensus on this as it will affect any > outstanding patches. What I've explained previously is trying to prevent that. The problem will be backports.
In case this is useful, I use this script for rebases: https://git.sr.ht/~jajm/dotfiles/tree/master/item/.local/lib/git-custom-commands/git-bz-rebase It resets the tree to where master/main was when the patch was submitted (uses bugzilla api), then apply the patches on top of it, and start rebase. It shouldn't be too hard to add a tidy step in between
I think it's worth the pain once (and for all) one question for me is what happens to all bugfixes waiting for testing & QA, theoretically the should all be tidy or not? if not they maybe should also be titied before automatically?
IIUC we are loosing more time constantly due to: - making tidying commits themselves - QA having to find which code section is the QA script complaining about if the QAer is fixing it themselves - same for the submitter for fixing it - another round trip of fail QA and back to signed off - the conflicts these commits cause with other submitted patches on close parts of the code when those patches are not tidied yet - and the backporting issues when there is a follow-up ticket built on tidied code but whose tidy commit had to be skipped. (so 2 bugs touching the same code and the tidy of 1st one wasn't kept to avoid conflict but now that causes conflicts when backporting the 2nd bug) All this, which is a constant burden vs the time lost to - take one massive round of conflicts with the submitted patches pool - and the backporting hassle significant for like 2 cycles. (stable and oldstable RMaints will need to ask a lot of rebasing help otherwise it will be impossible to keep up) So unless something makes the scale tilt the other way, let's grit our teeth and go for it! :) ----- To facilitate fixing conflicts on submitted patches, there might be this technique: - list the files giving conflicts - abort the failed apply/rebase operation - rebase the patches on the latest commit before the tidy patch (a big reason to tidy pl, js, tt, etc at the same time) - tidy the files that conflicted (the exact tidying commands should be somewhere easy to be found when needed) - commit - squash with the commit containing the actual changes - when there are multiple patches, they would all need to be squashed together :/ (any way around this?) - rebase on main to apply the great tidying patches and the rest - hopefully there shouldn't be conflicts (to be confirmed ^^") except regular legit ones And maybe that could be a base for the script to ease rebases. Hopefully there is a better strategy? (either for a script of manual step) So people with pending patches have the easiest time to get them though the great tidying. -------- @all Is there anything important that should be changed in the .perltidyrc and .prettierrc.js ? It's now or never. (we don't have a .tt tidying tool already, do we?) ------- (In reply to Jonathan Druart from comment #0) > I am suggesting to have patches for 25.05 that will tidy our perl (pl, pm, > t), javascript (js, ts, vue) and template (tt, inc) files. When testing Bug 38524 and it's dependencies, I don't recall prettier particularly complaining about the various ts and vue files I tested. So our config looks good. Because as ts and vue files are recent, prettier shouldn't have much to say if it's config is in line with out recent practices. So no issue foreseen there 👍 This is opposed to ESLint. Which raises a lot of issues in our ts and vue files. So the config doesn't match out recent practices and both need to be reconciled because it's not clear which of the two is right for us. So good thing it's only about Prettier. @all That's still an occasion to make important changes to the prettier config if needed. Note that Prettier should be on a recent version. Because on main, without Bug 38149, prettier is on whatever arbitrary version was latest when it was installed on one's dev env. Since it was not in package.json. --------- (In reply to Katrin Fischer from comment #13) > For info: this RM runs xt tests before pushing :) Should it be explicitly in the RM process? That's the closest thing I've found: https://wiki.koha-community.org/wiki/Release_management#Checking_for_regressions But it's not about running the whole xt/ tests. But maybe a pre-push hook might be enough for the RM. Or it's better to run all xt/ ? ------------------ (In reply to Jonathan Druart from comment #14) > The idea (not tested yet and needs to be confirmed/improved/discussed) is to: > 0. have a patch set for bug 12345 that has been written before the tidy > commit(s) > 1. run "the rebase script" > a. checkout commit before the tidy bug > b. apply patches from 12345 one-by-one > c. run the tidy script on each of the commit > d. rebase and keep changes from 12345: git checkout --theirs (or git > rebase -Xtheirs) > 2. Attach the rebased version of the patches c. is not possible. If it tidies the 1st commit, then it wouldn't be able to apply the 2nd anymore. The start point of the diff would have changed. That's why in my attempt earlier in this message, I had to resolve to tidy only after applying all commits and squash all :( Then apply the tidy bug. Wait, thinking about it again, there might be a way: a. checkout commit before the tidy bug b. apply the 1st patch c. tidy, commit and squash d. in a temporary branch (starting like step a.): apply the first two commits e. tidy, commit and squash f. now it should work to do `git merge --squash temp-branch;git commit` (and find how to reuse the commit message of commit 2) --squash takes the diff of two branches and prepares it to be committed. All this to have only the changes from commit 1 to commit 2 as if they where both tidied when authored. Otherwise commit 2 directly would conflict or rollback part of tidying of commit 1. And likely create invalid code. Like when tidying spreads stuff on multiple lines. 💥 Then repeat with a temporary branch having commit 1 + 2 + 3 and then tidy and squash. Any less contrived way to get the same result? 😵💫😵💫😵💫 ------------------------ (In reply to Mark Hofstetter from comment #16) > one question for me is what happens to all bugfixes waiting for testing & > QA, theoretically the should all be tidy or not? > > if not they maybe should also be titied before automatically? They are targeting the current code. So their diffs are from a non tidy codebase. Even if they would contain tidying changes (often it's in the last commit so the first commits don't have tidying) the starting point of the diff would have changed. So when trying to apply to the future tidied codebase git won't be able to match where to apply.
> @all Is there anything important that should be changed in the .perltidyrc and .prettierrc.js ? > It's now or never I don't think that tidying the whole codebase now prevents us to change the rules later (we'll just have another "tidy all" commit at the same time, which will probably be much smaller, depending on the rules changed). Anyway if we're considering changing the .perltidyrc I would advocate for adding -novalign (disabling vertical alignment). Vertical alignment may have some benefits, but it often requires changing code unrelated to the patch, making the patch bigger and harder to read. And when you have one long key/variable name it moves other values away from their key/variable name, making the code harder to read. I also dislike how spaces are added everywhere between parenthesis, but it's a minor inconvenience.
We shouldn't see 'tidy' commits. Those should be FQA most of the time. And the author should deal with tidying and squashing. Sometimes the tidy is a bit more complex and justifies an extra commit, but that's the exception I'd say. It would be great to have it all automated. But we really need to make sure everyone is using the same perltidy version and config \o/ I still hate it that KTD tries to manipulate my git clone on startup BTW.
(In reply to Tomás Cohen Arazi (tcohen) from comment #19) > I still hate it that KTD tries to manipulate my git clone on startup BTW. Either it's automatic, or we expect devs to do it (and update it). They don't.
(In reply to Tomás Cohen Arazi (tcohen) from comment #19) > We shouldn't see 'tidy' commits. Those should be FQA most of the time. And > the author should deal with tidying and squashing. > > Sometimes the tidy is a bit more complex and justifies an extra commit, but > that's the exception I'd say. Personally, I prefer having a separate tidy commit, since it makes it clear that the change to the existing code is due only to the tidy. I find it makes it easier to review code, especially on the Bugzilla UI, which is typically my first step for code review. > I still hate it that KTD tries to manipulate my git clone on startup BTW. Yeah I don't love it either.
(In reply to Jonathan Druart from comment #20) > (In reply to Tomás Cohen Arazi (tcohen) from comment #19) > > I still hate it that KTD tries to manipulate my git clone on startup BTW. > > Either it's automatic, or we expect devs to do it (and update it). They > don't. I mean the fact that it's difficult to enforce use of local git hooks makes me think that git hooks probably aren't the right way to do a number of the things we do. That said, they're probably one of the only tools we have available with our current workflow.
Created attachment 175488 [details] [review] Bug 38664: Fix acqui/z3950_search
Comment on attachment 175488 [details] [review] Bug 38664: Fix acqui/z3950_search Wrong bug!
Created attachment 175664 [details] [review] Bug 38664: Fixes for list and some tools templates tools/automatic_item_modification_by_age.tt tools/batchMod-del.tt tools/batchMod-edit.tt tools/batch_delete_records.tt tools/batch_extend_due_dates.tt tools/batch_record_modification.tt tools/cleanborrowers.tt tools/export.tt tools/import_borrowers.tt tools/inventory.tt tools/marc_modification_templates.tt tools/overduerules.tt tools/picture-upload.tt tools/quotes.tt tools/stage-marc-import.tt tools/stockrotation.tt tools/upload-images.tt tools/viewlog.tt virtualshelves/shelves.tt
(In reply to Owen Leonard from comment #25) > Created attachment 175664 [details] [review] [review] > Bug 38664: Fixes for list and some tools templates > > tools/automatic_item_modification_by_age.tt > tools/batchMod-del.tt > tools/batchMod-edit.tt > tools/batch_delete_records.tt > tools/batch_extend_due_dates.tt > tools/batch_record_modification.tt > tools/cleanborrowers.tt > tools/export.tt > tools/import_borrowers.tt > tools/inventory.tt > tools/marc_modification_templates.tt > tools/overduerules.tt > tools/picture-upload.tt > tools/quotes.tt > tools/stage-marc-import.tt > tools/stockrotation.tt > tools/upload-images.tt > tools/viewlog.tt > virtualshelves/shelves.tt On the remote branch, thanks! Adjusted the bug number (bug 38714).
Slightly related (esp if all code is touched): What do you think about moving all *.pm modules into `lib` (as is standard for CPAN dists). This would IMO make a lot of tooling and searching much easier, and would eg also make it easier to mount the local dev code into a docker container. (I guess this should be a distinct ticket?)
(In reply to Thomas Klausner from comment #27) > Slightly related (esp if all code is touched): What do you think about > moving all *.pm modules into `lib` (as is standard for CPAN dists). This > would IMO make a lot of tooling and searching much easier, and would eg also > make it easier to mount the local dev code into a docker container. (I guess > this should be a distinct ticket?) Probably a distinct ticket, but I'm always +1 for re-organising our source code.
(In reply to David Cook from comment #28) > (In reply to Thomas Klausner from comment #27) > > Slightly related (esp if all code is touched): What do you think about > > moving all *.pm modules into `lib` (as is standard for CPAN dists). This > > would IMO make a lot of tooling and searching much easier, and would eg also > > make it easier to mount the local dev code into a docker container. (I guess > > this should be a distinct ticket?) > > Probably a distinct ticket, but I'm always +1 for re-organising our source > code. Bug 28589.
Current status and updates will be there: https://koha-hedgedoc.servers.llownd.net/Bug-38664_Tidy-the-whole-codebase
This is ready for testing: https://gitlab.com/joubu/Koha/-/commits/bug_38664