Thanks for sending this link. Could we merge that code with QA code we already pushed into our codebase somehow ?
-----Original Message----- From: Koha-devel <koha-devel-boun...@lists.koha-community.org> On Behalf Of Jonathan Druart Sent: Monday, December 5, 2022 2:56 PM To: koha-devel <koha-devel@lists.koha-community.org> Subject: Re: [Koha-devel] Good enough? Existing hooks can be found at https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwiki.koha-community.org%2Fwiki%2FTips_and_tricks%23Hooks&data=05%7C01%7Cm.de.rooy%40rijksmuseum.nl%7C5d76e33c09514528c67808dad6c87f27%7C635b05eb66c748e1a94fb4b05a1b058b%7C0%7C0%7C638058453872270550%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=bxdii78avAYEIcecc8o0tQZ4v7atwVTkmbJMYPe1Dek%3D&reserved=0 Le lun. 5 déc. 2022 à 11:34, Marcel de Rooy <m.de.r...@rijksmuseum.nl> a écrit : > > Could you provide more info on those git hooks? A wiki URL? > > Marcel > > -----Original Message----- > From: Koha-devel <koha-devel-boun...@lists.koha-community.org> On > Behalf Of Jonathan Druart > Sent: Friday, December 2, 2022 3:43 PM > To: koha-devel <koha-devel@lists.koha-community.org> > Subject: [Koha-devel] Good enough? > > Hi devs, > > I was wondering... How good is your "good enough"? > It's a question I often ask myself, when writing patches or QAing > yours: is it good enough to be into Koha? It does not have to be perfect or > it may never reach master, but it must meet certain standards. > > When I was RM I tried to put that limit quite high. Not necessarily by asking > the author for improving the follow-up patches, but also by adding the > missing bits myself. > > There are various types of "good enough", depending on what we look > at: good enough to be reviewed, good enough to be tested, to be put in > production, get feedback, try an idea, etc. > > Most of the time, what is driving the limit is answering the questions "Is > it maintainable?" / "Is it future proof?". Making sure the code you are > writing won't be broken inadvertently is very important and must be part of > the reflection. > > Katrin asked the QA team members what were our plans for 23.05. In my opinion > we should enforce this "be future proof" concept. Writing code is easy, > especially in Koha (yes it is!). Writing maintainable and robust code, > following our guidelines is a bit harder. That's why we have a QA process > that tries to catch inconsistencies or edge cases you may have missed. > But I think we can be even more rigorous, and aim for better > standards. We can elevate our ambitions and do better. When we see the > number of new enhancements we are releasing every 6 months, it shows > well that writing code is definitely not a problem. However sometimes > developers are tempted to abandon their work once they are pushed to > master. Pushed does not mean "done". Providing bug fixes, following-up > with patches when needed, fixing CI jenkins, etc. is part of job > (/fun) > > As a Koha developer for a long time now, I know how frustrating it can be to > be asked for follow-ups/rewrite/tests to have our patches stamped with the > precious PQA mark. But from the other point of view (RM, RMaints, QA team), I > also know it's very frustrating when you are in charge of the release and you > do not get the appropriate follow-up work once it's pushed to master. > > There are some easy steps to write/review better patches. All have > been discussed already several times, but that can be enforced even > more: > 1. Perltidy (!) This is really a very trivial step. Please perltidy > your code. There are hundreds of commits that have been pushed in the > last months that are not tidy (alignment, indentation, lines too long, > etc.) This can easily be configured in your IDE! [1] > > 2. Provide clean code. As said it's not necessarily easy, but the QA team and > RM are supposed to know if the code is clean regarding Koha guidelines. If > the code is not clean, don't PQA, don't push. Either clean yourself, or ask > the original author of the patch to do it (explaining to them how it can be > improved ofc). > > 3. Squash! I have been away for a couple of months and had to read the git > history to know what I missed. And it was really hard to follow what was > going on. First of all, we are not consistent: the commit message must tell > what the patch is doing, not what the bug was (if you are writing a bug fix). > Then, there are way too many follow-ups: > tidiness, indentation fix, typo, spelling, etc. All those tiny follow-ups > could be squashed into the original patch. We don't need unnecessary tons of > entries in our git log for that. For instance, I usually add a "JD Amended > patch: perltidy" for instance when I tidy the original patch, to keep track > of the modification. Squash can be done by the original author, the QAer, the > RM. So yes, you are losing one commit in the stats but the git log is easy to > read! > We could have an "Amended-by" marker if we really want to add credit on the > dashboard (and/or release notes). > > 4. Run tests. Don't wait for Jenkins to fail. This is valid for the author > and QA. Anticipate the failures by running more tests. If you are modifying > C4::Circulation, then run prove on t/db_dependent/Circulation*, not only > Circulation.t. It will help you catch edge cases. > When something is pushed, track down jenkins failures that could be caused by > your patches. > > 6. Be strict if you are QAing. Each QA member has their own "good enough", > and the RM as well (either relying on the QAer or providing a full review). > But QA must fail if the code is old Koha style code, or not "good enough". > > 7. Provide support for failing tests, fix things you broke. The QA team will > be more comfortable with your patches if you show them you are providing > support for your stuff. > It's not because it's pushed that you don't have any more efforts to make. > Provide follow-up patches you promised, provide bug fixes, etc. > We don't have a good way to keep track of such demands, which does not make > tracking easier for devs, QA and RM. Any suggestions? > > 8. QA team MUST NEVER* pass QA a change that is not covered by tests, never. > You should not provide change to modules without tests! > * almost never... > > 9. Stick to existing patterns. We should not have different ways to do the > same thing. We should not have different places where a code is doing the > same thing. Ask for help or advice on the list or IRC before you start > coding. We will be happy to guide you. Even if you are a regular Koha > developer it's not always easy to be aware of the latest master changes. > We will tell you what's the current good practice, or point you to examples > you could reuse for what you want to implement. > > 10. CI should drive the pushes. No more push if CI is not green. The more we > wait the harder it is to track down the origin of the problem. > Last cycle some jobs have been red for months, and we released > 22.11.00 with D10, D11, D12 marked unstable... > > What will I do next cycle? > All of that, and more. I will track down jenkins failures and responsibilize > developers telling them when they break tests (and won't fix them anymore as > I have been doing for years). > I will raise on the bug reports what could have been improved. Yes, read that > I will be even more annoying (to put it politely) than before. > > I've noticed that the pre-commit git hook on the wiki has been broken for > more than 3 years. And also caught some core developers that do not have it > in place. I am relying on it to keep Vue files tidy so it's important to have > it set up properly. I am planning to force its usage for ktd users [2]. > Adding more checks to it will help us to catch inconsistencies from the > beginning. > > To summarize, writing code is cheap, maintaining code is way more expensive! > It is easier to get the attention of developers before the patches are pushed > to master than after, so we could be more ambitious and ask more. > > For discussion :) > > Cheers, > Jonathan > > [1] If you are using vim, open ~/vimrc, add > vmap <F8> :!perltidy -q<CR> > Reload vim, select code in visual mode [2] > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitl > ab.com%2Fkoha-community%2Fkoha-misc4dev%2F-%2Fissues%2F59&data=05% > 7C01%7Cm.de.rooy%40rijksmuseum.nl%7C5d76e33c09514528c67808dad6c87f27%7 > C635b05eb66c748e1a94fb4b05a1b058b%7C0%7C0%7C638058453872270550%7CUnkno > wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL > CJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=AkLM4JATxwG4L8CcLpZ7z34oM6GCEN > 49uAo3dt4Sw18%3D&reserved=0 > _______________________________________________ > Koha-devel mailing list > Koha-devel@lists.koha-community.org > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist > s.koha-community.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fkoha-devel&d > ata=05%7C01%7Cm.de.rooy%40rijksmuseum.nl%7C5d76e33c09514528c67808dad6c > 87f27%7C635b05eb66c748e1a94fb4b05a1b058b%7C0%7C0%7C638058453872270550% > 7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik > 1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=aqN6n2Z2kvsodCb1MlLsNiA > AomgcvKr6fB7tCNNdBrg%3D&reserved=0 > website : > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww. > koha-community.org%2F&data=05%7C01%7Cm.de.rooy%40rijksmuseum.nl%7C > 5d76e33c09514528c67808dad6c87f27%7C635b05eb66c748e1a94fb4b05a1b058b%7C > 0%7C0%7C638058453872270550%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDA > iLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sd > ata=3vEx8LIuZAkid78L%2FF15H58ECkQQu4m4PO7R4Tnbot0%3D&reserved=0 > git : > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit. > koha-community.org%2F&data=05%7C01%7Cm.de.rooy%40rijksmuseum.nl%7C > 5d76e33c09514528c67808dad6c87f27%7C635b05eb66c748e1a94fb4b05a1b058b%7C > 0%7C0%7C638058453872270550%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDA > iLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sd > ata=5bMenLRZpyj4HYRx%2FExY83Mwe%2F8bpaLvlJvG3jLcBCg%3D&reserved=0 > bugs : > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs > .koha-community.org%2F&data=05%7C01%7Cm.de.rooy%40rijksmuseum.nl%7 > C5d76e33c09514528c67808dad6c87f27%7C635b05eb66c748e1a94fb4b05a1b058b%7 > C0%7C0%7C638058453872270550%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD > AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&s > data=iT5Hl4dNO8NQXuQZmJmkdLr1pXv0k3TBGK8NceA82iE%3D&reserved=0 _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.koha-community.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fkoha-devel&data=05%7C01%7Cm.de.rooy%40rijksmuseum.nl%7C5d76e33c09514528c67808dad6c87f27%7C635b05eb66c748e1a94fb4b05a1b058b%7C0%7C0%7C638058453872270550%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=aqN6n2Z2kvsodCb1MlLsNiAAomgcvKr6fB7tCNNdBrg%3D&reserved=0 website : https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.koha-community.org%2F&data=05%7C01%7Cm.de.rooy%40rijksmuseum.nl%7C5d76e33c09514528c67808dad6c87f27%7C635b05eb66c748e1a94fb4b05a1b058b%7C0%7C0%7C638058453872270550%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=3vEx8LIuZAkid78L%2FF15H58ECkQQu4m4PO7R4Tnbot0%3D&reserved=0 git : https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.koha-community.org%2F&data=05%7C01%7Cm.de.rooy%40rijksmuseum.nl%7C5d76e33c09514528c67808dad6c87f27%7C635b05eb66c748e1a94fb4b05a1b058b%7C0%7C0%7C638058453872270550%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=5bMenLRZpyj4HYRx%2FExY83Mwe%2F8bpaLvlJvG3jLcBCg%3D&reserved=0 bugs : https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.koha-community.org%2F&data=05%7C01%7Cm.de.rooy%40rijksmuseum.nl%7C5d76e33c09514528c67808dad6c87f27%7C635b05eb66c748e1a94fb4b05a1b058b%7C0%7C0%7C638058453872270550%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=iT5Hl4dNO8NQXuQZmJmkdLr1pXv0k3TBGK8NceA82iE%3D&reserved=0 _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : https://www.koha-community.org/ git : https://git.koha-community.org/ bugs : https://bugs.koha-community.org/
