jmcnamara / excel-writer-xlsx

Perl module to create Excel XLSX files.
https://metacpan.org/pod/distribution/Excel-Writer-XLSX/lib/Excel/Writer/XLSX.pm
Other
100 stars 51 forks source link

fix regression in 1bf0068 when set_column() is called with $last_col==0 #279

Closed sherrardb closed 1 year ago

sherrardb commented 1 year ago

good day, there seems to have been a regression introduced in 1bf0068 when calling set_column() with $last_col set to zero.

  my $workbook = Excel::Writer::XLSX->new( $fh );
  $worksheet->set_column( 1, 0, 35 ); # does not work
  $worksheet->set_column( 'B:B', 35 ); # works
  $worksheet->set_column( 1, 1, 35 ); # probably works but did not check
  $workbook->close();

the docs indicate that this syntax should still work, even though the A1 syntax is suggested. this seems to be a fairly simply fix, but please forgive me if i am overlooking some context or intent.

sorry i could not find a relevant unit test to update as all of the ones i looked at in t/regression/set_column* and t/regression/xlsx_files/set_column* seem to use the A1 syntax.

My automatically generated system details are as follows:

Perl version   : 5.028001
OS name        : linux
Module versions: (not all are required)
                 Excel::Writer::XLSX        1.09
                 Spreadsheet::WriteExcel    (not installed)
                 Archive::Zip               1.68
                 XML::Writer                0.900
                 IO::File                   1.45
                 File::Temp                 0.2311
jmcnamara commented 1 year ago
$worksheet->set_column( 1, 0, 35 ); # does not work

That isn't valid syntax. It should really be like the other example with the same first and last column:

  $worksheet->set_column( 1, 1, 35 ); # probably works but did not check

However, I seemed to have put a fix in at some point (2010!) to stop people shooting themselves in the foot I presume because it could crash Excel. So it is probably best to use the correct syntax and not rely on that failsafe.

Nevertheless, there is a regression and your fix is valid so I will merge it. Thanks.

sherrardb commented 1 year ago
$worksheet->set_column( 1, 0, 35 ); # does not work

That isn't valid syntax. It should really be like the other example with the same first and last column:

i only discovered this when maintaining some in-house code written by another developer. i found the syntax non-intuitive, but then i confirmed in the perldoc that it is acceptable, or at least that was my interpretation of the following: https://github.com/jmcnamara/excel-writer-xlsx/blob/0b746a3bfd5ad78121ead38632d387b9d91a79fe/lib/Excel/Writer/XLSX.pm#L2284

thanks for getting to this so quickly.

jmcnamara commented 1 year ago

i found the syntax non-intuitive, but then i confirmed in the perldoc that it is acceptable, or at least that was my interpretation of the following

Wow. I can't believe I documented that exception. I really should have just converted it to an error. It seems that I ported it over from Spreadsheet::WriteExcel code from 2007!.

Anyway, if it is in the docs then I will let it stand even if it looks wrong. Thanks, for the debug and fix.

sherrardb commented 1 year ago

indeed, it is almost impossible to deprecate anything in open source code once it is in the wild. unless you want to take this as an opportunity to start the soft deprecation. :-) all the best