always clear range todos so that evaluation after error is stable

[igheorghita]

• [x] closes #128
• [x] tests passed
• [x] passes tox

I just wrapped the evaluation of the ranges in range_todos in the function _process_gen_graph in a try/finally. In the finally statement we clear range_todos so that if we can't evaluate some range, it doesn't get trapped in range_todos forever. Previously, the range with the error would remain there, so every time you'd try to evaluate a cell that wasn't in the cell_map you'd error out the first time evaluate was called. This hopefully makes the order of evaluation irrelevant.

If you prefer the warning solution still, let me know. We could alternatively log a warning here, but I figured a complete solution would be nicer...

I haven't added any tests - would be happy to add some if you think it's necessary! It resolves my example in #128 at least. Could have test that compares 1) the values of evaluated cells before one attempts to evaluate an unimplemented function and 2) the values afterwards, as a sanity check.

[stephenrauch]

Would this code also handle your use case?:

# calc the values for ranges
while self.range_todos:
    self._evaluate_range(self.range_todos.pop())

[igheorghita]

The only issue with that is that if you try to evaluate a cell whose evaluation depends on say, two other cells with unimplemented functions, then range_todos will still be stuck with a bad range.

I updated my test case with such an example: switch_test.xlsx

For example, if you run

model = ExcelCompiler('switch_test.xlsx')
model.evaluate('E3')

At this point model.range_todos = ['Sheet1!E1:G1', 'Sheet1!A2:C2'] - your code gets rid of 'A1:C1' but not 'E1:G1'.

# so then this results in an error during the first evaluation attempt
model.evaluate('A3')

[stephenrauch]

I am unable to duplicate this error. Here is the test case that works, but if I understand your bug report correctly, this test case should not work.

def test_unknown_function_ws(fixture_xls_copy):
    compiler = ExcelCompiler(fixture_xls_copy('switch_test.xlsx'))

    # 1. This works properly
    assert compiler.evaluate('Sheet1!A3') == 'string'

    # 2. This doesn't work as expected
    from pycel.excelformula import UnknownFunction
    with pytest.raises(UnknownFunction):
        compiler.evaluate('Sheet1!E3')  # gives an error as expected

    # then immediately after try
    assert compiler.evaluate('Sheet1!A3') == 'string'

    # If you now try to evaluate A3 again, it works, because it's already in model.cell_map
    assert compiler.evaluate('Sheet1!A3') == 'string'

It would very helpful if you could construct a test case that fails without your changes.

[igheorghita]

Ah, what you have will work because you first evaluate A3 and this puts it in the cell_map, so when you evaluate A3 after E3 _process_gen_graph won't get called. But if you don't evaluate A3 before E3, it shouldn't work.

This should run into an AssertionError without the changes:

def test_unknown_function_ws(fixture_xls_copy):
    compiler = ExcelCompiler(fixture_xls_copy('switch_test.xlsx'))

    from pycel.excelformula import UnknownFunction
    with pytest.raises(UnknownFunction):
        compiler.evaluate('Sheet1!E3')  

    assert compiler.evaluate('Sheet1!A3') == 'string'

[codecov-commenter]

Codecov Report

Merging #132 (6aaa09a) into master (c0c720f) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #132   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines         3734      3740    +6     
  Branches       905       908    +3     
=========================================
+ Hits          3734      3740    +6     

Impacted Files	Coverage Δ
src/pycel/excelcompiler.py	100.00% <100.00%> (ø)
src/pycel/excellib.py	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c0c720f...6aaa09a. Read the comment docs.