Bug 9144 - bulkmarcimport.pl - Problem identifying errors
Summary: bulkmarcimport.pl - Problem identifying errors
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic record staging/import (show other bugs)
Version: master
Hardware: All All
: P1 - high trivial (vote)
Target Milestone: ---
Assignee: Vitor Fernandes
QA Contact: Paul Poulain
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-26 10:47 UTC by Vitor Fernandes
Modified: 2013-05-23 06:24 UTC (History)
3 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
BUG 9144 (863 bytes, patch)
2012-11-26 11:24 UTC, Vitor Fernandes
Details | Diff | Splinter Review
Bug 9144 - bulkmarcimport.pl - Problem identifying errors (1.04 KB, patch)
2012-12-07 12:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9144 - bulkmarcimport.pl - Problem identifying errors (1.09 KB, patch)
2012-12-12 08:05 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Vitor Fernandes 2012-11-26 10:47:21 UTC
The bulkmarcimport.pl uses replace instead of changing line, which difficults the track of the errors found in the migration.

Solution: replace \r to \n
Comment 1 Vitor Fernandes 2012-11-26 11:24:33 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-12-07 12:40:01 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-12-07 12:41:37 UTC
We use \n for newlines everywhere else in Koha, so this is a sensible change.
Comment 4 Paul Poulain 2012-12-12 08:04:36 UTC
QA comment: one letter patch, passes QA !
Comment 5 Paul Poulain 2012-12-12 08:05:03 UTC
Created attachment 14027 [details] [review]
Bug 9144 - bulkmarcimport.pl - Problem identifying errors

Replace \r with \n for newline in output for bulkmarcimport.pl

\r is the newline character for OS X, \n is the newline character
for Linux.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Comment 6 Jared Camins-Esakov 2012-12-12 13:27:00 UTC
I am inclined to push this patch, since I agree with Vitor's assessment that it would probably be easier to spot errors when using \n than when using \r. However, the sign off and QA appear to be predicated on the idea that this patch increases consistency, which it does not do. In light of this, I would request that Kyle and Paul confirm whether they would like their sign off and QA (respectively) to stand.
Comment 7 Kyle M Hall 2012-12-12 14:44:15 UTC
(In reply to comment #6)
> I am inclined to push this patch, since I agree with Vitor's assessment that
> it would probably be easier to spot errors when using \n than when using \r.
> However, the sign off and QA appear to be predicated on the idea that this
> patch increases consistency, which it does not do. In light of this, I would
> request that Kyle and Paul confirm whether they would like their sign off
> and QA (respectively) to stand.

I stand by my SO, for ease of readability. I suppose if we want full consistency, we would need to find every \r and \n in Koha and change them to \r\n.
Comment 8 Paul Poulain 2012-12-17 16:50:18 UTC
(In reply to comment #6)
> I am inclined to push this patch, since I agree with Vitor's assessment that
> it would probably be easier to spot errors when using \n than when using \r.
> However, the sign off and QA appear to be predicated on the idea that this
> patch increases consistency, which it does not do. In light of this, I would
> request that Kyle and Paul confirm whether they would like their sign off
> and QA (respectively) to stand.

Jared, I don't understand what you're saying. (ie= I don't see where I said this patch improves consistency)

back to passed QA, this patch is OK for me
Comment 9 Jared Camins-Esakov 2012-12-17 16:54:06 UTC
This patch has been pushed to master.

(Paul, you didn't say explicitly that you were QAing the patch with the idea that it improved consistency, but that was implied by the commit message and I wanted to make sure you wholly approved)
Comment 10 Chris Cormack 2012-12-17 21:43:58 UTC
Pushed to 3.10.x will be in 3.10.1