fix order of evaluation test

[igheorghita]

Sorry to bother you again with this... but I just realized the test I wrote didn't actually do what we wanted because I didn't set ws.formula_attributes! That means the test would have passed even without the fix. Now it should be good though. (Without the changes, the first assertion would have failed, and with the while loop/pop solution you proposed, the second assertion would have failed.)

I also figured there's no reason to make this test dependent on the switch function being unimplemented.

[codecov-commenter]

Codecov Report

Merging #134 (db6b04f) into master (3f586c3) will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #134   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines         3757      3757           
  Branches       912       912           
=========================================
  Hits          3757      3757           

---

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 3f586c3...db6b04f. Read the comment docs.

[stephenrauch]

I had tested your test case before merging, and so I am not understanding what was felt to be missing. More importantly at this point, I refactored same because I was implementing SWITCH() and the reliance on switch was, as you noted, not ideal.

So if the changes are still needed, please update the refactored test case to reflect the needed extra functionality.

Also, thanks for taking the time to run this thing to ground and submit the PR. That needed finally was a very non-trivial thing to figure out.

[igheorghita]

Just looked through test_evaluate_after_range_eval_error. My only point is that this test would have passed without the changes made in #132, i.e., if you revert the lines back to

for range_todo in reversed(self.range_todos):
    self._evaluate_range(range_todo)
self.range_todos = []

the test will still pass. Something like this wouldn't have though:

def test_evaluate_after_range_eval_error():
    wb = Workbook()
    ws = wb.active
    ws['A1'], ws['B1'], ws['C1'] = 0, 1, 0
    ws['A2'] = '=UNKNOWN(A1:C1,0,FALSE,1,TRUE)'
    ws.formula_attributes['A2'] = {'t': 'array', 'ref': "A2:C2"}
    ws['A3'] = 'hello'
    ws['A4'] = '=UNKNOWN(A1:C1,1,FALSE,0,TRUE)'
    ws.formula_attributes['A4'] = {'t': 'array', 'ref': "A4:C4"}
    ws['A5'] = '=AND(A2,A4)'

    excel_compiler = ExcelCompiler(excel=wb)
    with pytest.raises(UnknownFunction):
        excel_compiler.evaluate('A2')
    assert excel_compiler.evaluate('A3') == 'hello'

    excel_compiler = ExcelCompiler(excel=wb)
    with pytest.raises(UnknownFunction):
        excel_compiler.evaluate('A5')
    assert excel_compiler.evaluate('A3') == 'hello'

When you read from an actual Excel file, openpyxl will save the formula attributes and with SWITCH it'll keep track of the array it spills into, but when I wrote the test I forgot to set the attributes. Basically my point is that if you replace UNKNOWN with SWITCH in the test case, it won't actually behave like the "analogous" Excel file (unless you set ws.formula_attributes like I did above). Does that make sense? You probably know more about this than me, but for reference.

And why this relates to the behavior in #128: whether or not the formula_attributes are set with {'t': 'array', ...} will totally change what's in range_todos - you can check!

Hope this made sense!

[stephenrauch]

So clearly, I am not paying enough attention. Thanks for the cogent explanation. If you want to go ahead and update the PR with that code, please do. Or I can take care of it, if needed. Thanks again for your patience with me...

Cheers.

[igheorghita]

Oh no worries, sorry for not getting it right the first time