14:02:47 #startmeeting Development IRC meeting 24 October 2018 14:02:47 Meeting started Wed Oct 24 14:02:47 2018 UTC. The chair is kidclamp. Information about MeetBot at http://wiki.debian.org/MeetBot. 14:02:47 Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. 14:02:47 The meeting name has been set to 'development_irc_meeting_24_october_2018' 14:02:53 #topic Introductions 14:03:02 kidclamp: oki super 14:03:12 #info Nick Clemens, ByWater Solutions 14:03:24 #chair Joubu 14:03:24 Current chairs: Joubu kidclamp 14:03:24 #info Fridolin Somers, Biblibre 14:03:28 #info Martin Renvoize - PTFS-E, Uk 14:03:34 Ish 14:03:46 #info Tomas Cohen Arazi, Theke Solutions 14:03:47 Waiting at the school gates 14:03:53 @info Barton Chittenden, ByWater Solutions 14:03:53 barton: Error: The command "info" is available in the Factoids and RSS plugins. Please specify the plugin whose command you wish to call by using its name as a command before "info". 14:03:55 #info Josef Moravec, Municipal Library Usti nad Orlici, Czech Republic 14:04:01 #info Claire Gravely, BSZ, Germany 14:04:05 oops. 14:04:09 #info Mark Tompsett 14:04:11 #info Barton Chittenden, ByWater Solutions 14:04:15 #info Thomas Dukleth, Agogme, New York City 14:04:36 #info Owen Leonard, Athens County Public Libraries, USA 14:04:43 #info Jonathan Druart 14:05:13 #info Jesse Maseto, BWS 14:05:21 #info Kyle M Hall, ByWater Solutions 14:05:29 #link https://wiki.koha-community.org/wiki/Development_IRC_meeting_24_October_2018 14:05:48 full house :-) 14:05:54 Wow 14:05:59 kidclamp and ashimema distracted me and I forgot to fill the agenda with tons of topics 14:06:07 #topic Announcements 14:06:14 Lol.. sorry Joubu 14:06:15 you still can joubu, just let us know to reload 14:06:24 any announcements> 14:07:24 last call for announcements... 14:07:31 I am planning a "what's on in koha-devel" email soon 14:07:43 https://annuel2.framapad.org/p/What_s_on_in_koha-devel # if you want to your topics 14:08:16 #info Joubu is working on a 'what's on in koha devel' - please add topics to keep this email going 14:08:31 #link https://annuel2.framapad.org/p/What_s_on_in_koha-devel What's On email topic 14:08:47 also, bookmark https://frama.link/koha_bz_rel_18_11_candidate - but I already sent an email about that 14:09:06 #info Check the 18.11 bug list too 14:09:16 #link https://frama.link/koha_bz_rel_18_11_candidate 18.11 bugs 14:09:26 and "sql modes", but later 14:09:30 #topic Update from the Release manager (18.11) 14:09:33 That's me! 14:09:56 I am working on cleaning out the queue - ES has seen some movement, focus there to get things in before slush is appreciated 14:10:02 slush is coming fast 14:10:16 follow the tag as above for things we need in 18.11 14:10:36 I am excited for the release :-) we did good work, thank you all 14:10:39 Scary fast 14:11:02 what is slush? 14:11:02 rumour has it slush is coming fast 14:11:27 enhancements not in PQA will not be considered for the release 14:11:29 Comes before freeze 14:11:36 ok 14:11:39 thanks 14:11:39 freeze means no more enhancements 14:12:19 November 2 - Feature slush - no enhancement/new features that have not passed QA will be considered for release 14:12:20 Will all Passed QA make it into the next release? 14:12:22 November 9 - Feature freeze - only bugfixes will pushed after this - 1st draft of release notes 14:12:25 November 16 - String Freeze - 2nd draft of release notes 14:12:27 November 27 - Release 14:12:54 mtompset: they are still subject to review - i iwll do my best barring issues 14:13:00 thanks Joubu 14:13:18 okay, thank you, kidclamp. :) 14:13:30 that's all i have for right now 14:14:02 #topic Updates from the Release Maintainers 14:14:11 ashimema? 14:14:11 hmmm... ashimema is RMaint for 18.05 ? 14:14:35 ashimema said he would be late 14:14:44 fridolin? 14:14:44 fridolin is indeed, but maybe not on Rmain 18.11 14:14:48 yep 14:14:53 I juste released 14:14:57 https://koha-community.org/koha-17-11-11-release/ 14:15:08 We made release 14:15:18 And I have some catching up to do now 14:15:24 I'm late on 18.05.x for around 10 bugs I will catchup 14:15:25 But otherwise, nothing from me 14:15:43 #info ashimema and frido are releasing releases and being cool 14:15:52 * fridolin asked help for rebase of Bug 21385 14:15:52 04Bug http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21385 major, P2, ---, martin.renvoize, RESOLVED FIXED, Vendor search: Item count is incorrectly updated on partial receive 14:16:00 ashimema: is on it 14:16:19 oh why resolved 14:16:25 Yup 14:16:34 #topic Updates from the QA team 14:17:02 juste to finish, Plugins with CSS and JS are in 17.11.x 14:17:08 enjoy :) 14:17:10 cait joubu marcelr jajm khall tcohen alex_a josef_moravec 14:17:19 fridolin++ 14:17:27 fridolin++ 14:17:32 kidclamp: sql modes? 14:17:55 Joubu: leave that for the begining of the next release 14:18:05 I figured you would bring up now 14:18:42 tcohen: I do not understand what you mean 14:18:58 are you telling we should not discuss it before the release? 14:19:18 no, I mean the changes in Koha::Object 14:19:36 this is important, needs to be discussed! 14:19:48 ok 14:19:49 so 14:19:53 "strict sql modes" 14:20:05 I am going to create a page on the wiki to explain the situation 14:20:16 what we did, where we are, and what need to be done 14:20:25 also the problems we are facing 14:20:48 to me the priority is to stop the regressions, at least the ones tests can catch 14:21:06 and so you should focuss on bug 21613 14:21:06 04Bug http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21613 normal, P5 - low, ---, jonathan.druart, Signed Off , Turn strict SQL modes on for tests 14:21:12 nothing else for now 14:22:10 Joubu++ 14:22:32 #info Joubu will create a wiki page for the strcit sql modes issue 14:22:52 #link http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21613 Bug to add strict mode to tests to prevent regressions 14:22:52 04Bug 21613: normal, P5 - low, ---, jonathan.druart, Signed Off , Turn strict SQL modes on for tests 14:23:57 #topic General development discussion (trends, ideas, ...) 14:24:15 ashimema around? 14:24:38 ashimema, said he was going to run late, I think... scrolls up... 14:25:08 " I'll be late to the dev meeting.. school run duties 14:25:08 please accept this as my apologies" 14:25:36 jsut checking, I ma not sure what is needed for trailers discussion, or just to highlight them 14:25:53 #link https://wiki.koha-community.org/wiki/Commit_messages#Other_trailers Other git trailers 14:25:58 I'm sorta around but in the playground 14:26:16 Git trailer s was a hang over from last meeting.. 14:26:28 Wasn't proposed by me.. I just like the idea 14:26:30 are these *other* trailers a recent thing? 14:26:53 It's all documented anyways.. just needed highlighting to sleeve new about it 14:26:58 Yes kohaputti 14:27:34 cool 14:27:36 Especially the Sponsored-by one which I intend to make more use of for release notes 14:27:38 should not they be added by RM (automatically 14:27:39 ) 14:27:41 I like them, but I cannot say whether the implementation is correct 14:28:02 Just one question: would they change they ... "git bz apply; test; git so; git bz attach" workflow? 14:28:15 Oops.. change THE... 14:28:27 If not, I don't care. :) 14:28:52 mtompset, I think not because sign-off for example can come after of before, it really doesn't matter or does it? 14:29:03 * ashimema now has child so disappears again 14:29:19 will it get messy if these different types of trailers are not sorted? 14:30:07 I don't see them causing any issues, they should come before the signoff ones I suppose, but is an easy fix 14:30:13 Could end up as an RM thing I suppose Joubu 14:30:30 But if it does would be good to script as much as we can 14:30:53 * mtompset chuckles, "Automate everything!" 14:31:23 I think we can try them, right? It doesn't hurt? 14:32:36 agreed 14:32:56 Would the patch submitter be encouraged to add also "Reported-by"? Or just RM 14:33:31 Reported-by can be scripted 14:33:52 I don't think we are suggesting these are required either, just encouraged and allowed? 14:34:11 "Co-authored-by" we usually follow-up instead 14:34:19 "Mentored-by" cannot be scripted 14:34:30 "Thanks-to" must be done manually too 14:34:33 Yup 14:34:34 Not required 14:34:57 Will there be a list of "valid trailer tags" on the wiki? 14:34:57 moving on? 14:35:02 I used Mentored-by the other day.. 14:35:10 we will certainly not use "Mentored-by" much 14:35:10 would Reported-by be a bit redundant since it is already in bugzilla? 14:35:31 kohaputti, but that would allow for it to transfer to release notes easier. 14:35:43 mtompset: a link has been sent before 14:35:49 okay. 14:36:02 I didn't follow the conversation closely. 14:36:23 mtompset, but can't we extract that from bugzilla with some automated fashion? 14:36:53 Whatever... moving on, like kidclamp said. :) 14:36:53 does each bug say to which version the patches has been pushed? 14:36:56 okay 14:37:35 these are all good questions :-) I think followup on the email thread and we can develop guidelines et.c if these become widely used or we have a need 14:37:43 #topic Review of coding guidelines 14:37:57 #info Using nested subroutines (as discussed in Bug 19893) 14:37:57 04Bug http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 enhancement, P5 - low, ---, glasklas, Needs Signoff , Alternative optimized indexing for Elasticsearch 14:38:26 What is your opinion on using nested sub like: https://github.com/colinsc/koha/compare/master...PTFS-Europe:bug_19893_es_fast#diff-9b2d7c9428adb3f8fd6236eec1297cebR341 ? 14:39:06 I think if it is not an closure, i.e. it doesn't refer to outer subroutine it should be moved in top level 14:39:44 on the other hand if it is closure, i.e. refers to something in the outer subroutine I think it is fine for it to be a nested subroutine 14:39:54 My question is why isn't this classed? It's written like a class. 14:40:41 I really dislike nested routines. 14:40:56 mtompset, that could be a good way to do it but I don't think we should focus on this specific piece of code right now? 14:41:33 mtompset, what are the drawbacks of nested routines in your opinion? 14:41:56 I myself noticed it makes it hard to follow the code because I cannot differentiate the to routines from each other 14:42:06 the two routines* 14:42:07 maybe moved to Koha::FieldMappings? 14:42:21 our code is not cleanly indented across the project. it makes it harder to eyeball. 14:43:21 Anything longer than 3 lines (which is an arbitrary okay inline size) should be it's own class. 14:43:34 ^it's^its^ 14:43:51 class? You mean subroutine? 14:44:05 I'm against it 14:44:07 I think it does affect readability, quick research seems to indicate it is not 'good' perl unless it is a closure 14:44:36 It could be a coderef and be a bit more readable 14:45:30 * ashimema would like to read more into it before just throwing it out.. see what perl best practice and higher order perl suggests about it 14:45:30 I think why it at least for me affects the readability is because it makes the outer function bigger and there there is more things I need to keep in mind at the same time 14:45:47 what is the benefit of nesting it? 14:45:50 class as in OO Perl, like Joubu suggested a Koha::FieldMappings or something. 14:45:52 if it was a functor, and we were generating multiple functions to be passed around, maybe but this looks obfuscated as-is, with no clear win 14:46:09 kidclamp, the benefit is when you want to refer to something in the outer function, like some variable 14:46:11 I'm also not sure it scopes that way he's expecting 14:46:34 tcohen, yes, a functor (if I am understanding your word use) would be okay with a coderef. 14:47:17 quick reading about it now, and I am against it as it is used here... 14:47:19 yes, but you might need to generate functions, but hey, this is not the case 14:47:23 kohaputti, if you want to refer to an outer variable, then you shoudl have passed it as a parameter. 14:47:37 ashimema: yes, the scoping looks like one of the problems 14:47:40 -- hashref preferably. :) 14:48:22 I would like to see some refactoring here - move to better place, as Joube mentioned for example¨ 14:48:34 the main issue is that you cannot unit test it 14:48:46 -- which you can with a class. :) 14:49:11 unit_tests++ :) 14:49:25 unittests++ :) 14:49:29 tests++ :) 14:49:49 regressions_tests++ :) 14:49:51 I think we can vote? 14:49:51 mtompset, yeah I think it can be passed as a parameter too but I guess there is some reason for closure (https://perldoc.perl.org/perlref.html). 14:49:55 unitte_ests++ 14:50:23 I think we should not rule out closures completely? but just such nesting where it is actually not a closure 14:50:24 oleonard++ # love your spelling. 14:50:51 No, no... closures of a SMALL size (which is why I threw out an arbitrary 3 lines) would be okay. 14:51:03 mtompset: sts is obviously not a word. 14:51:16 kohaputti: agreed, but this doesn't seem to be a closure as understood, this is a named internal subroutine 14:51:24 mtompset, do you mean with a closure a subroutine that refers to outer subroutines variable? 14:51:45 kidclamp, yes it is not closure in that example 14:52:13 I think we can add the rule and consider exceptions if warranted 14:52:16 perhaps... but very limited scope and use. Generally be avoided. 14:52:17 so to put it more clearly: I don't want named internal subroutines, but closure are okay 14:52:22 we are all reasonable people :-) 14:53:05 should we develop a coding guideline? 14:53:18 kidclamp, we have one..? 14:53:21 I do not think it's needed 14:53:31 Possibly, but perhaps not until a few of us have researched further 14:53:52 kidclamp, you meant for this issue? 14:53:57 It's a pretty rare occurrence to come across it 14:54:34 yeah, just wondering if we needed action, or just discussion for this one bug? 14:55:19 Well, what do we do when someone else writes a large internal routine like this again? 14:55:43 Just the discussion I think.. I'd really like to see that bug moving on but if I'm honest I don't think it's ready for 18.11 myself.. not as is. 14:55:45 QA will fail it 14:56:06 That's why I think we need a bit more thought before we guideline it 14:56:09 -- unless it is a newbie QA'r? ;) 14:56:10 I think we could deal with this now, or if you want have some time to think about it and decide in the next meeting? 14:56:29 Right now QA picked it up as a code smell and brought up discussions.. that was the right thing to do 14:56:58 if we guideline it, the guideline needs to be flexible and not-rigid, but generally encourage a particular coding style. 14:57:25 definitely code smell here. 14:57:30 okay. no guideline for now, will someone respond on the bug to say we don't like it? then we can move on 14:57:54 agreed 14:58:24 * ashimema is overwhelmed at the moment.. anyone else fancy commenting 14:58:34 #info we do not like nested subroutines - we should avoid them 14:58:50 #info Proposal add guideline SQL13: DB Updates - when preparing DB updates you must use skeleton.perl as the base and ensure updates are idempotent (multiple runs should not cause errors) 14:58:54 * ashimema certainly wants to re-read a few chapters before he would comment 14:59:39 I definitely like the idea of idempotent. :) 14:59:43 I can forward this discussion to the author also, and then work together on refactoring it 14:59:45 kidclamp: .sql is useful for "INSERT IGNORE INTO systempreferences" single query 15:00:11 Thanks, Joubu. I was going to mention that. :) 15:00:19 idenpotent is a guideline already 15:00:34 Nested subroutines when used appropriately can produce less complexity and easier readability. 15:00:40 They're already meant to be idempotent 15:00:58 It's just not in that guidelines page but in a dB guidelines one 15:01:23 Yes,m I just want to make it easily visible and spelled out, it is threaded through other pages on the wiki 15:01:25 Joubu, where is that in the guidelines? 15:01:40 the wiki alsmo mentions .sql files. but I don't want to see those anymore 15:01:42 :-) 15:01:42 thd, I will research on that 15:02:04 essentially now the RM cleans up the update if it is not in the right format, and I don't want to , because I am likely to mess it up 15:02:13 if it's not in the wiki it's in my head 15:02:22 Ah, I see it: https://wiki.koha-community.org/wiki/Database_updates#Idempotent 15:02:41 just want it easily spelled out and obvious so we all know where it is and I can fail QA and make the author fix :-) 15:02:42 ... but the could be an endless recursion trap more easily. 15:04:00 bye 15:04:30 So...you think it is okay to add? or you think we don't need it? 15:05:03 I think we should centralize it in the coding guidelines. :) 15:05:06 or, "Updates should be written as directed on the wiki at https://wiki.koha-community.org/wiki/Database_updates" 15:05:09 I think it can be added, also to the README in installer/data/mysql/atomicupdate 15:05:15 But not necessarily require the .perl thing. 15:05:28 I want to require the perl 15:05:54 mtompset, what would be a case where skeleton.perl cannot be used? 15:06:01 kidclamp, when INSERT IGNORE works? 15:06:32 kohaputti, it can always be used. It's just that a person doing it, might just be more comfortable providing just the SQL. 15:06:36 it still gets wrapped in perl when moved to updatedatabase.pl - I just want that tobe the work of the author to avoid copying issues 15:06:51 and having to requote sql statements 15:07:08 use q|| instead of quote and you are done ;) 15:07:17 I prefer q{} 15:07:18 lazy kidclamp 15:07:29 unless the statement has pipes in it joubu 15:07:52 lazy developers? 15:08:01 someone is being lazy in either casee 15:08:33 Joubu, write a script to turn .sql atomicupdates to .perl for kidclamp. ;) 15:08:36 well doesn't the RM already has enough things to do? Maybe we should provide the script to do .perl file from just the bare SQL 15:08:53 mtompset, exactly 15:09:49 the sekelton.perl is already there, I just don't see a reason to not use it 15:10:01 but this is not getting the support i hoped :-) 15:11:22 kidclamp, whenever I do an atomic update with a .perl, I'll remember to use your name in vain. ;) 15:11:33 lol 15:11:46 mtompset, have you come a cross anybody who was uncomfortable doing the update with the skeleton.perl? 15:12:02 I would suggest to update the wiki and tell people we prefer a .perl even for single statement 15:12:07 Me. I rather just get the SQL right. :) 15:12:15 but we should not FQA for that reason 15:12:29 I can support Joubu's suggestion. :) 15:12:35 fair engouh, but I will sigh a lot -- jsut know you make me sigh :-) 15:12:42 mtompset, so your worry is that the perl wrapper creates bugs? 15:12:48 Can you make a video of that? ;) 15:13:12 kidclamp: rely on QA team to move .sql to .perl 15:14:03 at that point I feel lazy, anyways, I think we can then just add a link tothe update page ont eh coding guidelines and call it a day? 15:14:05 kohaputti, nope. Just worried it will do something else magical. Don't trust those inner workings. EEEEEVIL! ;) 15:14:28 mtompset: it is just a wrapper - the rm moving it or you putting it there are the same 15:14:55 Is anyone else arguing that this is a bad idea? 15:15:24 mtompset++ on getting the SQL right! 15:15:27 It's not a BAD idea... just requiring it... Guideline it... 15:15:33 Okay, so... RM says do it. 15:15:38 not mandate it. 15:16:01 unless there are objections I will add the link to the coding guidelines page, and add to that page that skeleton is STRONGLY encouraged 15:16:09 no guideline 15:16:46 okay. 15:16:47 If the RM would convert it to perl then there is nobody reviewing it. I like it more when the author does it so there is the review part included 15:17:06 valid point, kohaputti. 15:17:10 kohaputti++ 15:17:32 nothing worse than rolling a package which is broken. 15:18:00 kidclamp's name will be used less in vain. 15:18:03 cannot happen, tests will fail 15:18:18 anyways, I had enoguh discussion, and no objections 15:18:20 Because there are no vanilla SQL atomicupdates in reality then I think we should only allow .perl atomicupdates 15:18:23 #topic Set time of next meeting 15:18:58 November 7, 21 UTC? 15:19:32 #info Next meeting: 7 November 2018, 21 UTC 15:19:45 #endmeeting