Description
Jonathan Druart
2024-05-23 07:38:31 UTC
Created attachment 167078 [details] [review] Bug 36935: Remove warning if file parsing is successful This patch removes the logged warning if a file parse is successful. Test plan: 1) prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t 2) The output will have a logged message - 'EOF - End of data in parsing input stream' 3) Apply patch 4) Repeat step 1 5) The warning will be gone errno 2012 means successful? Apparently so! https://metacpan.org/pod/Text::CSV_XS#DIAGNOSTICS Created attachment 167080 [details] [review] Bug 36935: Remove warning if file parsing is successful This patch removes the logged warning if a file parse is successful. Test plan: 1) prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t 2) The output will have a logged message - 'EOF - End of data in parsing input stream' 3) Apply patch 4) Repeat step 1 5) The warning will be gone Signed-off-by: David Nind <david@davidnind.com> (In reply to Matt Blenkinsop from comment #3) > Apparently so! > https://metacpan.org/pod/Text::CSV_XS#DIAGNOSTICS I am not sure, this is not clear for me. use Text::CSV_XS; open(my $fh, "<", '/tmp/file') or die $!; my $csv = Text::CSV_XS->new( { sep_char => ',', binary => 1, allow_loose_quotes => 1 } ); my @lines = @{$csv->getline_all($fh)}; close($fh); my ( $cde, $str, $pos ) = $csv->error_diag(); my $error = $cde ? "$cde, $str, $pos" : ""; use Data::Printer colored => 1; say p @lines; /tmp/file contains one,two,three 1,2,3 4,5,6 7,8,9 run the script and get no error open my $fh, "<", \$file_content or die; This must die with the reason! The bare minimum is 'die $!' Or add a newline to the input file or so ? Created attachment 167088 [details] [review] Bug 36935: Remove warning if file parsing is successful This patch removes the logged warning if a file parse is successful. Test plan: 1) prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t 2) The output will have a logged message - 'EOF - End of data in parsing input stream' 3) Apply patch 4) Repeat step 1 5) The warning will be gone Created attachment 167089 [details] [review] Bug 36935: Add a 'die' message to file parsing Is this error handling covered by a test? Not currently, I'll add one. Patch incoming If I force the error handling to throw an error in a unit test we will get a warning in the output which is what we're trying to avoid. Now that we're identifying the end of file we could throw a Koha::Exception rather than a warning? Use Test::Warn (warning_is for instance) if you want to test that a warning will be displayed in the log. An exception would be better ofc. Created attachment 167099 [details] [review] Bug 36935: Add unit test for error handling prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t Bug 36942 raised for throwing an exception, will try and have a look at that when I have some time Created attachment 167130 [details] [review] Bug 36935: Remove warning if file parsing is successful This patch removes the logged warning if a file parse is successful. Test plan: 1) prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t 2) The output will have a logged message - 'EOF - End of data in parsing input stream' 3) Apply patch 4) Repeat step 1 5) The warning will be gone Signed-off-by: David Nind <david@davidnind.com> Created attachment 167131 [details] [review] Bug 36935: Add a 'die' message to file parsing Signed-off-by: David Nind <david@davidnind.com> Created attachment 167132 [details] [review] Bug 36935: Add unit test for error handling prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t Signed-off-by: David Nind <david@davidnind.com> Created attachment 168326 [details] [review] Bug 36935: Remove warning if file parsing is successful This patch removes the logged warning if a file parse is successful. Test plan: 1) prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t 2) The output will have a logged message - 'EOF - End of data in parsing input stream' 3) Apply patch 4) Repeat step 1 5) The warning will be gone Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 168327 [details] [review] Bug 36935: Add a 'die' message to file parsing Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 168328 [details] [review] Bug 36935: Add unit test for error handling prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Thanks Matt, always good to make the tests a little cleaner :) Pushed for 24.11! Well done everyone, thank you! Backported to 24.05.x for upcoming 24.05.06 Depends on Bug 34788 not in 23.11.x Affects the log files and tests, so no changes required to the manual. |