Fix: Add OpenReader options to GetRows

[vivekkairi]

PR Details

The Options struct passed to OpenReader function is not passed in the GetRows which results in passing the options separately resulting in duplication

Description

Passing Options to OpenReader should automatically pass the same Options in the child function call as it is done initially. Currently the OpenReader options are not used in GetRows function, which causes inconsistency and leads to duplication.

Related Issue

#1815

Motivation and Context

It solves the duplicate need of passing Options if it's already passed in OpenReader

How Has This Been Tested

Tested locally on my machine with multiple excel files and the output it gives satisfies the requirement

Types of changes

• Docs change / refactoring / dependency upgrade
• [ X] Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• [X ] I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• [ X] All new and existing tests passed.

[xuri]

Currently, the OpenReader options are not used in the GetRows function, the GetRows function just uses the same data structure Options as the OpenReader function to specify the RawCellValue. I agree with making the cell value reading functions inherit the Options settings of the OpenReader function, and we need to consider following 2 conditions:

1. If Options have also been passed when calling the cell value reading functions, its priority should be higher than OpenReader and override its settings, so the code needs to be changed to:

-opts = append(opts, *f.options)
+opts = append([]Options{*rows.f.options}, opts...)

2. The following cell reading functions also need to inherit the Options settings:

• Columns
• GetCols
• Rows
• GetCellValue

Based on the above two points, to avoid append options duplicated, I suggest changing the getOptions function to support inheriting the Options settings of the OpenReader function, just like this:

-func getOptions(opts ...Options) *Options {
-    options := &Options{}
+func (f *File) getOptions(opts ...Options) *Options {
+    options := f.options

3. The unit tests covered these changes

[vivekkairi]

Currently, the OpenReader options are not used in the GetRows function, the GetRows function just uses the same data structure Options as the OpenReader function to specify the RawCellValue. I agree with making the cell value reading functions inherit the Options settings of the OpenReader function, and we need to consider following 2 conditions:

1. If Options have also been passed when calling the cell value reading functions, its priority should be higher than OpenReader and override its settings, so the code needs to be changed to:

-opts = append(opts, *f.options)
+opts = append([]Options{*rows.f.options}, opts...)

2. The following cell reading functions also need to inherit the Options settings:

• Columns
• GetCols
• Rows
• GetCellValue

Based on the above two points, to avoid append options duplicated, I suggest changing the getOptions function to support inheriting the Options settings of the OpenReader function, just like this:

-func getOptions(opts ...Options) *Options {
-    options := &Options{}
+func (f *File) getOptions(opts ...Options) *Options {
+    options := f.options

3. The unit tests covered these changes

I've done the code changes, the current tests are passing. Could you guide me through the unit tests once?

[vivekkairi]

Fixed, also removed multiple times options fetch in CalcCellValue

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9cbe3b6) 99.20% compared to head (5bb98ff) 99.20%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1816   +/-   ##
=======================================
  Coverage   99.20%   99.20%           
=======================================
  Files          32       32           
  Lines       23851    23852    +1     
=======================================
+ Hits        23662    23663    +1     
  Misses        101      101           
  Partials       88       88           

Flag	Coverage Δ
unittests	99.20% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[xuri]

LGTM, thanks for your contribution.

[vivekkairi]

Hey @xuri

I am using v2 version of excelize, do I need to send a separate PR for that?

[xuri]

The next version will be released based on the master branch. You can upgrade to the master branch code by go get -u github.com/xuri/excelize/v2@master before that.