Bug 19190 - Silly calculation of average time in touch_all scripts
Summary: Silly calculation of average time in touch_all scripts
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-28 09:59 UTC by Marcel de Rooy
Modified: 2019-10-14 19:55 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19190: Silly calculation of average time in touch_all scripts (2.00 KB, patch)
2017-08-28 10:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19190: [Follow-up] Do not rely on the return of ModItem (1.40 KB, patch)
2017-08-28 10:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19190: [Follow-up] Do not rely on the return of ModItem (1.42 KB, patch)
2017-09-22 19:10 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19190: Silly calculation of average time in touch_all scripts (2.06 KB, patch)
2017-10-02 08:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19190: [Follow-up] Do not rely on the return of ModItem (1.46 KB, patch)
2017-10-02 08:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19190: Silly calculation of average time in touch_all scripts (2.08 KB, patch)
2017-10-06 17:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19190: [Follow-up] Do not rely on the return of ModItem (1.47 KB, patch)
2017-10-06 17:48 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-08-28 09:59:06 UTC
When you want to calculate average time, do not divide count by time :)
Comment 1 Marcel de Rooy 2017-08-28 10:18:23 UTC
Created attachment 66550 [details] [review]
Bug 19190: Silly calculation of average time in touch_all scripts

When you want to calculate average time, do not divide count by time :)

Test plan:
Run the script with a where condition and verbose option and see that
the average time is meaningful.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2017-08-28 10:18:29 UTC
Created attachment 66551 [details] [review]
Bug 19190: [Follow-up] Do not rely on the return of ModItem

touch_all_items looks at the return of ModItem to determine if the
operation was successful. But ModItem does not return a meaningful
value. This patch puts the ModItem call in an eval and looks at $@.

Test plan:
Run touch_all_items with a where condition and verbose option.
Put print 1/0; at the end of ModItem.
Run touch_all_items again. You should see: ERROR WITH ITEM xxx !!!!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 David Bourgault 2017-09-22 19:10:59 UTC
Created attachment 67332 [details] [review]
Bug 19190: [Follow-up] Do not rely on the return of ModItem

touch_all_items looks at the return of ModItem to determine if the
operation was successful. But ModItem does not return a meaningful
value. This patch puts the ModItem call in an eval and looks at $@.

Test plan:
Run touch_all_items with a where condition and verbose option.
Put print 1/0; at the end of ModItem.
Run touch_all_items again. You should see: ERROR WITH ITEM xxx !!!!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 4 Marcel de Rooy 2017-10-02 08:33:21 UTC
Created attachment 67499 [details] [review]
Bug 19190: Silly calculation of average time in touch_all scripts

When you want to calculate average time, do not divide count by time :)

Test plan:
Run the script with a where condition and verbose option and see that
the average time is meaningful.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 5 Marcel de Rooy 2017-10-02 08:33:25 UTC
Created attachment 67500 [details] [review]
Bug 19190: [Follow-up] Do not rely on the return of ModItem

touch_all_items looks at the return of ModItem to determine if the
operation was successful. But ModItem does not return a meaningful
value. This patch puts the ModItem call in an eval and looks at $@.

Test plan:
Run touch_all_items with a where condition and verbose option.
Put print 1/0; at the end of ModItem.
Run touch_all_items again. You should see: ERROR WITH ITEM xxx !!!!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 6 Marcel de Rooy 2017-10-02 08:34:09 UTC
(In reply to David Bourgault from comment #3)
> Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Thanks, David.
I added your signoff line also to the first patch.
Comment 7 Nick Clemens 2017-10-06 17:48:01 UTC
Created attachment 67752 [details] [review]
Bug 19190: Silly calculation of average time in touch_all scripts

When you want to calculate average time, do not divide count by time :)

Test plan:
Run the script with a where condition and verbose option and see that
the average time is meaningful.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Nick Clemens 2017-10-06 17:48:04 UTC
Created attachment 67753 [details] [review]
Bug 19190: [Follow-up] Do not rely on the return of ModItem

touch_all_items looks at the return of ModItem to determine if the
operation was successful. But ModItem does not return a meaningful
value. This patch puts the ModItem call in an eval and looks at $@.

Test plan:
Run touch_all_items with a where condition and verbose option.
Put print 1/0; at the end of ModItem.
Run touch_all_items again. You should see: ERROR WITH ITEM xxx !!!!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Jonathan Druart 2017-10-09 19:18:44 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 10 Marcel de Rooy 2017-12-04 14:01:54 UTC
Backport ?
Comment 11 Fridolin Somers 2017-12-11 15:53:32 UTC
Pushed to 17.05.x, will be in 17.05.07