xlcalculator icon indicating copy to clipboard operation
xlcalculator copied to clipboard

XLRange does not support all types of ranges.

Open strichter opened this issue 4 years ago • 9 comments

build_defined_names() still fails for me. Now there is a different error, since XLRnage does not support all types of ranges. Valid ranges include:

A1
A1:Z9
A:A
1:1
Sheet!A1
Sheet!A1:Z9
Sheet!A:A
Sheet!1:1

For me it fails specifically for the A:A and '1:1' style ranges, since your code requires always both parts of the coordinate.

strichter avatar May 18 '20 09:05 strichter

Both parts and a sheet, yes.

I think this fix might be "big".

bradbase avatar May 18 '20 11:05 bradbase

I think I may need more detail on your use case.

Sheet context is a challenge. Currently cell addresses and range addresses are used as keys in dicts for ranges and cells. As xlcalculator supports multiple worksheets the worksheet needs to be specified in the address of the cell or the range unless it's a defined name. The worksheet can be omitted on the range definition in Excel but is expected to become qualified with a worksheet as the range gets loaded into the Model().

It appears to be impossible to create a defined name that omits the worksheet part of the range address for A:A and 1:1. eg; they all get recorded as something like Sheet1!A:A or Sheet1!1:1.

To support range definitions with the current implementation of the model object, A:A and 1:1 will need to assume the worksheet is the one upon which they occur. If those ranges appear with an unqualified worksheet in a formula, the worksheet context will be that of the formula.

So we really only need to support ranges like Sheet1!A:A and Sheet!1:1 while adding the worksheet part of the address as the range is getting loaded into a Model().

Is there a circumstance in your use cases where ranges that specify a worksheet is not appropriate? eg; are you likely to want to call get_cell_value() or set_cell_value() on ranges A:A or 1:1 instead of Sheet1!A:A or Sheet1!1:1?

bradbase avatar May 18 '20 12:05 bradbase

For defined ranges, the Sheet name is required, so I always use Sheet1!A;A. But since the error was in the generic XLRange parser, I was pointing out all valid options.

strichter avatar May 18 '20 13:05 strichter

Is this the traceback you were faced with?

====================================================================== ERROR: test_address (tests.xlcalculator_types.range_test.TestXLRange)

Traceback (most recent call last): File "range_test.py", line 60, in test_address xlrange_05 = XLRange('Sheet1!1:1', 'Sheet1!1:1') File "", line 4, in init File "range.py", line 69, in cells range_cells = XLRange.cell_address_infill(cells, sheet=self.sheet) File "range.py", line 96, in cell_address_infill start_column, start_row = [_f for _f in re.split('([A-Z$]+)', start_cell_address) if _f] ValueError: not enough values to unpack (expected 2, got 1)


Ran 2 tests in 0.002s

bradbase avatar May 18 '20 14:05 bradbase

Yes

strichter avatar May 18 '20 15:05 strichter

I have the bug on the run but it's 1:30am. I doubt I'm going to solve it tonight without screwing something else up and/or it'll be a bad solution.

Over the weekend I bumped into issues with worksheet scoping elsewhere in the code where you've not been able to exercise it yet. I'll try and solve them individually but it seems while writing xlcalculator I've left a lot for "later".

Apologies for holding you up but I'll need to come back to this after some rest.

bradbase avatar May 18 '20 15:05 bradbase

I am good as I have overwritten the method to not read any named ranges as I do not need them for calculations. (But of course I would like to not have to create a custom class at all. :-)

strichter avatar May 19 '20 10:05 strichter

Great to hear you can sidestep it.

In relation to the collection of issues you've raise, I'm impressed that you're using xlcalculator in a way I hadn't considered. Also I am grateful you're raising issues to move things to make that easier (actually, possible).

Obviously addresses in these forms need to be supported. There's no justification required, but yes you should not need to make custom classes for this purpose.

Today I've been working on it and have started to support range address form 1:1. This has highlighted a need to work on file loading performance and is now triggering problems with memory management (OOM). On my machine it takes around 8 minutes to load an Excel file with a single formula Sheet2!A1 =Sheet1!1:1. This time is way too long as there are only 16,384 cells in columns from A to XFD. I'm genuinely mystified at how I can OOM with that volume of elements in a dict so I'm obviously doing something bizarre. I found a couple of silly things which have changed the nature of the problem but have not yet killed all the bugs.

bradbase avatar May 19 '20 14:05 bradbase

At the end of today I have moved the OOMs and, as expected, have now broken some tests. I expect to solve them tomorrow.

bradbase avatar May 20 '20 11:05 bradbase