<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body>A new request with request id 6915 has been created by koha-devel-request@lists.koha-community.org. Short info on the request is : <br><br>Title : Koha-devel Digest, Vol 205, Issue 3<br>Category : <br>Description : <div>Send Koha-devel mailing list submissions to<br> koha-devel@lists.koha-community.org<br><br>To subscribe or unsubscribe via the World Wide Web, visit<br> https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel<br>or, via email, send a message with subject or body 'help' to<br> koha-devel-request@lists.koha-community.org<br><br>You can reach the person managing the list at<br> koha-devel-owner@lists.koha-community.org<br><br>When replying, please edit your Subject line so it is more specific<br>than "Re: Contents of Koha-devel digest..."<br><br><br>Today's Topics:<br><br> 1. Good enough? (Jonathan Druart)<br> 2. Re: Good enough? (Julian Maurice)<br><br><br>----------------------------------------------------------------------<br><br>Message: 1<br>Date: Fri, 2 Dec 2022 15:42:39 +0100<br>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org><br>To: koha-devel <koha-devel@lists.koha-community.org><br>Subject: [Koha-devel] Good enough?<br>Message-ID:<br> <CAJzKNY43JEjN2ycSC8Xxni6VH1ZRzNaj=qfT-DP5=m-TVeQb6g@mail.gmail.com><br>Content-Type: text/plain; charset="UTF-8"<br><br>Hi devs,<br><br>I was wondering... How good is your "good enough"?<br>It's a question I often ask myself, when writing patches or QAing<br>yours: is it good enough to be into Koha? It does not have to be<br>perfect or it may never reach master, but it must meet certain<br>standards.<br><br>When I was RM I tried to put that limit quite high. Not necessarily by<br>asking the author for improving the follow-up patches, but also by<br>adding the missing bits myself.<br><br>There are various types of "good enough", depending on what we look<br>at: good enough to be reviewed, good enough to be tested, to be put in<br>production, get feedback, try an idea, etc.<br><br>Most of the time, what is driving the limit is answering the<br>questions "Is it maintainable?" / "Is it future proof?". Making sure<br>the code you are writing won't be broken inadvertently is very<br>important and must be part of the reflection.<br><br>Katrin asked the QA team members what were our plans for 23.05. In my<br>opinion we should enforce this "be future proof" concept. Writing code<br>is easy, especially in Koha (yes it is!). Writing maintainable and<br>robust code, following our guidelines is a bit harder. That's why we<br>have a QA process that tries to catch inconsistencies or edge cases<br>you may have missed.<br>But I think we can be even more rigorous, and aim for better<br>standards. We can elevate our ambitions and do better. When we see the<br>number of new enhancements we are releasing every 6 months, it shows<br>well that writing code is definitely not a problem. However sometimes<br>developers are tempted to abandon their work once they are pushed to<br>master. Pushed does not mean "done". Providing bug fixes, following-up<br>with patches when needed, fixing CI jenkins, etc. is part of job<br>(/fun)<br><br>As a Koha developer for a long time now, I know how frustrating it can<br>be to be asked for follow-ups/rewrite/tests to have our patches<br>stamped with the precious PQA mark. But from the other point of view<br>(RM, RMaints, QA team), I also know it's very frustrating when you are<br>in charge of the release and you do not get the appropriate follow-up<br>work once it's pushed to master.<br><br>There are some easy steps to write/review better patches. All have<br>been discussed already several times, but that can be enforced even<br>more:<br>1. Perltidy (!) This is really a very trivial step. Please perltidy<br>your code. There are hundreds of commits that have been pushed in the<br>last months that are not tidy (alignment, indentation, lines too long,<br>etc.) This can easily be configured in your IDE! [1]<br><br>2. Provide clean code. As said it's not necessarily easy, but the QA<br>team and RM are supposed to know if the code is clean regarding Koha<br>guidelines. If the code is not clean, don't PQA, don't push. Either<br>clean yourself, or ask the original author of the patch to do it<br>(explaining to them how it can be improved ofc).<br><br>3. Squash! I have been away for a couple of months and had to read the<br>git history to know what I missed. And it was really hard to follow<br>what was going on. First of all, we are not consistent: the commit<br>message must tell what the patch is doing, not what the bug was (if<br>you are writing a bug fix). Then, there are way too many follow-ups:<br>tidiness, indentation fix, typo, spelling, etc. All those tiny<br>follow-ups could be squashed into the original patch. We don't need<br>unnecessary tons of entries in our git log for that. For instance, I<br>usually add a "JD Amended patch: perltidy" for instance when I tidy<br>the original patch, to keep track of the modification. Squash can be<br>done by the original author, the QAer, the RM. So yes, you are losing<br>one commit in the stats but the git log is easy to read!<br>We could have an "Amended-by" marker if we really want to add credit<br>on the dashboard (and/or release notes).<br><br>4. Run tests. Don't wait for Jenkins to fail. This is valid for the<br>author and QA. Anticipate the failures by running more tests. If you<br>are modifying C4::Circulation, then run prove on<br>t/db_dependent/Circulation*, not only Circulation.t. It will help you<br>catch edge cases.<br>When something is pushed, track down jenkins failures that could be<br>caused by your patches.<br><br>6. Be strict if you are QAing. Each QA member has their own "good<br>enough", and the RM as well (either relying on the QAer or providing a<br>full review). But QA must fail if the code is old Koha style code, or<br>not "good enough".<br><br>7. Provide support for failing tests, fix things you broke. The QA<br>team will be more comfortable with your patches if you show them you<br>are providing support for your stuff.<br>It's not because it's pushed that you don't have any more efforts to<br>make. Provide follow-up patches you promised, provide bug fixes, etc.<br>We don't have a good way to keep track of such demands, which does not<br>make tracking easier for devs, QA and RM. Any suggestions?<br><br>8. QA team MUST NEVER* pass QA a change that is not covered by tests,<br>never. You should not provide change to modules without tests!<br>* almost never...<br><br>9. Stick to existing patterns. We should not have different ways to do<br>the same thing. We should not have different places where a code is<br>doing the same thing. Ask for help or advice on the list or IRC before<br>you start coding. We will be happy to guide you. Even if you are a<br>regular Koha developer it's not always easy to be aware of the latest<br>master changes.<br>We will tell you what's the current good practice, or point you to<br>examples you could reuse for what you want to implement.<br><br>10. CI should drive the pushes. No more push if CI is not green. The<br>more we wait the harder it is to track down the origin of the problem.<br>Last cycle some jobs have been red for months, and we released<br>22.11.00 with D10, D11, D12 marked unstable...<br><br>What will I do next cycle?<br>All of that, and more. I will track down jenkins failures and<br>responsibilize developers telling them when they break tests (and<br>won't fix them anymore as I have been doing for years).<br>I will raise on the bug reports what could have been improved. Yes,<br>read that I will be even more annoying (to put it politely) than<br>before.<br><br>I've noticed that the pre-commit git hook on the wiki has been broken<br>for more than 3 years. And also caught some core developers that do<br>not have it in place. I am relying on it to keep Vue files tidy so<br>it's important to have it set up properly. I am planning to force its<br>usage for ktd users [2]. Adding more checks to it will help us to<br>catch inconsistencies from the beginning.<br><br>To summarize, writing code is cheap, maintaining code is way more<br>expensive! It is easier to get the attention of developers before the<br>patches are pushed to master than after, so we could be more ambitious<br>and ask more.<br><br>For discussion :)<br><br>Cheers,<br>Jonathan<br><br>[1] If you are using vim, open ~/vimrc, add<br> vmap <F8> :!perltidy -q<CR><br>Reload vim, select code in visual mode<br>[2] https://gitlab.com/koha-community/koha-misc4dev/-/issues/59<br><br><br>------------------------------<br><br>Message: 2<br>Date: Fri, 2 Dec 2022 18:16:24 +0100<br>From: Julian Maurice <julian.maurice@biblibre.com><br>To: koha-devel@lists.koha-community.org<br>Subject: Re: [Koha-devel] Good enough?<br>Message-ID: <85c5041c-a3ee-db1c-b576-97e789193c14@biblibre.com><br>Content-Type: text/plain; charset=UTF-8; format=flowed<br><br>Hi,<br><br>It seems to me that several issues could be solved by having the CI run <br>sooner, so authors can have feedback as soon as possible. Something like <br>github's CI integration with pull requests would be amazing to detect <br>common mistakes early and stop wasting human time. We should know if <br>tests pass before pushing to master, not after.<br>We could check tidyness, commit message conventions, code coverage by <br>tests, ... all before another person have to look at it.<br><br>Also tests are not easy to run locally. They might pass on Jenkins, but <br>they do not on my local setup, so they are basically useless to tell me <br>if I broke something. It also makes writing new tests more difficult <br>than it should.<br><br>If I wanted to improve Koha developer experience, I would start with that.<br><br>My 2 cents.<br><br>Le 02/12/2022 à 15:42, Jonathan Druart a écrit :<br>> Hi devs,<br>> <br>> I was wondering... How good is your "good enough"?<br>> It's a question I often ask myself, when writing patches or QAing<br>> yours: is it good enough to be into Koha? It does not have to be<br>> perfect or it may never reach master, but it must meet certain<br>> standards.<br>> <br>> When I was RM I tried to put that limit quite high. Not necessarily by<br>> asking the author for improving the follow-up patches, but also by<br>> adding the missing bits myself.<br>> <br>> There are various types of "good enough", depending on what we look<br>> at: good enough to be reviewed, good enough to be tested, to be put in<br>> production, get feedback, try an idea, etc.<br>> <br>> Most of the time, what is driving the limit is answering the<br>> questions "Is it maintainable?" / "Is it future proof?". Making sure<br>> the code you are writing won't be broken inadvertently is very<br>> important and must be part of the reflection.<br>> <br>> Katrin asked the QA team members what were our plans for 23.05. In my<br>> opinion we should enforce this "be future proof" concept. Writing code<br>> is easy, especially in Koha (yes it is!). Writing maintainable and<br>> robust code, following our guidelines is a bit harder. That's why we<br>> have a QA process that tries to catch inconsistencies or edge cases<br>> you may have missed.<br>> But I think we can be even more rigorous, and aim for better<br>> standards. We can elevate our ambitions and do better. When we see the<br>> number of new enhancements we are releasing every 6 months, it shows<br>> well that writing code is definitely not a problem. However sometimes<br>> developers are tempted to abandon their work once they are pushed to<br>> master. Pushed does not mean "done". Providing bug fixes, following-up<br>> with patches when needed, fixing CI jenkins, etc. is part of job<br>> (/fun)<br>> <br>> As a Koha developer for a long time now, I know how frustrating it can<br>> be to be asked for follow-ups/rewrite/tests to have our patches<br>> stamped with the precious PQA mark. But from the other point of view<br>> (RM, RMaints, QA team), I also know it's very frustrating when you are<br>> in charge of the release and you do not get the appropriate follow-up<br>> work once it's pushed to master.<br>> <br>> There are some easy steps to write/review better patches. All have<br>> been discussed already several times, but that can be enforced even<br>> more:<br>> 1. Perltidy (!) This is really a very trivial step. Please perltidy<br>> your code. There are hundreds of commits that have been pushed in the<br>> last months that are not tidy (alignment, indentation, lines too long,<br>> etc.) This can easily be configured in your IDE! [1]<br>> <br>> 2. Provide clean code. As said it's not necessarily easy, but the QA<br>> team and RM are supposed to know if the code is clean regarding Koha<br>> guidelines. If the code is not clean, don't PQA, don't push. Either<br>> clean yourself, or ask the original author of the patch to do it<br>> (explaining to them how it can be improved ofc).<br>> <br>> 3. Squash! I have been away for a couple of months and had to read the<br>> git history to know what I missed. And it was really hard to follow<br>> what was going on. First of all, we are not consistent: the commit<br>> message must tell what the patch is doing, not what the bug was (if<br>> you are writing a bug fix). Then, there are way too many follow-ups:<br>> tidiness, indentation fix, typo, spelling, etc. All those tiny<br>> follow-ups could be squashed into the original patch. We don't need<br>> unnecessary tons of entries in our git log for that. For instance, I<br>> usually add a "JD Amended patch: perltidy" for instance when I tidy<br>> the original patch, to keep track of the modification. Squash can be<br>> done by the original author, the QAer, the RM. So yes, you are losing<br>> one commit in the stats but the git log is easy to read!<br>> We could have an "Amended-by" marker if we really want to add credit<br>> on the dashboard (and/or release notes).<br>> <br>> 4. Run tests. Don't wait for Jenkins to fail. This is valid for the<br>> author and QA. Anticipate the failures by running more tests. If you<br>> are modifying C4::Circulation, then run prove on<br>> t/db_dependent/Circulation*, not only Circulation.t. It will help you<br>> catch edge cases.<br>> When something is pushed, track down jenkins failures that could be<br>> caused by your patches.<br>> <br>> 6. Be strict if you are QAing. Each QA member has their own "good<br>> enough", and the RM as well (either relying on the QAer or providing a<br>> full review). But QA must fail if the code is old Koha style code, or<br>> not "good enough".<br>> <br>> 7. Provide support for failing tests, fix things you broke. The QA<br>> team will be more comfortable with your patches if you show them you<br>> are providing support for your stuff.<br>> It's not because it's pushed that you don't have any more efforts to<br>> make. Provide follow-up patches you promised, provide bug fixes, etc.<br>> We don't have a good way to keep track of such demands, which does not<br>> make tracking easier for devs, QA and RM. Any suggestions?<br>> <br>> 8. QA team MUST NEVER* pass QA a change that is not covered by tests,<br>> never. You should not provide change to modules without tests!<br>> * almost never...<br>> <br>> 9. Stick to existing patterns. We should not have different ways to do<br>> the same thing. We should not have different places where a code is<br>> doing the same thing. Ask for help or advice on the list or IRC before<br>> you start coding. We will be happy to guide you. Even if you are a<br>> regular Koha developer it's not always easy to be aware of the latest<br>> master changes.<br>> We will tell you what's the current good practice, or point you to<br>> examples you could reuse for what you want to implement.<br>> <br>> 10. CI should drive the pushes. No more push if CI is not green. The<br>> more we wait the harder it is to track down the origin of the problem.<br>> Last cycle some jobs have been red for months, and we released<br>> 22.11.00 with D10, D11, D12 marked unstable...<br>> <br>> What will I do next cycle?<br>> All of that, and more. I will track down jenkins failures and<br>> responsibilize developers telling them when they break tests (and<br>> won't fix them anymore as I have been doing for years).<br>> I will raise on the bug reports what could have been improved. Yes,<br>> read that I will be even more annoying (to put it politely) than<br>> before.<br>> <br>> I've noticed that the pre-commit git hook on the wiki has been broken<br>> for more than 3 years. And also caught some core developers that do<br>> not have it in place. I am relying on it to keep Vue files tidy so<br>> it's important to have it set up properly. I am planning to force its<br>> usage for ktd users [2]. Adding more checks to it will help us to<br>> catch inconsistencies from the beginning.<br>> <br>> To summarize, writing code is cheap, maintaining code is way more<br>> expensive! It is easier to get the attention of developers before the<br>> patches are pushed to master than after, so we could be more ambitious<br>> and ask more.<br>> <br>> For discussion :)<br>> <br>> Cheers,<br>> Jonathan<br>> <br>> [1] If you are using vim, open ~/vimrc, add<br>> vmap <F8> :!perltidy -q<CR><br>> Reload vim, select code in visual mode<br>> [2] https://gitlab.com/koha-community/koha-misc4dev/-/issues/59<br>> _______________________________________________<br>> Koha-devel mailing list<br>> Koha-devel@lists.koha-community.org<br>> https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel<br>> website : https://www.koha-community.org/<br>> git : https://git.koha-community.org/<br>> bugs : https://bugs.koha-community.org/<br><br>-- <br>Julian Maurice<br>BibLibre<br><br><br>------------------------------<br><br>Subject: Digest Footer<br><br>_______________________________________________<br>Koha-devel mailing list<br>Koha-devel@lists.koha-community.org<br>https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel<br>website : https://www.koha-community.org/<br>git : https://git.koha-community.org/<br>bugs : https://bugs.koha-community.org/<br><br><br>------------------------------<br><br>End of Koha-devel Digest, Vol 205, Issue 3<br>******************************************<br></div><br><br>NOTE: You are receiving this mail because, the Requester/Technician wanted you to get notified on this request creation.<br></body></html>