Let me know what your preference for tests is.

I fixed the failing typo check on master. If you rebase to that it will fix the failing CI check.

rebased

Could you add test case(s) for the second commit: "Handle non-monotonic si number".

Hi sorry for the delay.
I've rebased off master and added two fixups which adds tests for the two commits. I've verified that both tests fail before the change and now pass. I've also enabled the CI on my fork.

btw once you're happy with the pr, I'll do git rebase -i $(git merge-base HEAD upstream/master) --autosquash and squash the fixup commits so they're ready to merge unless you prefer using squash and merge on PRs.

btw once you're happy with the pr, I'll do git rebase -i $(git merge-base HEAD upstream/master) --autosquash and squash the fixup commits so they're ready to merge unless you prefer using squash and merge on PRs.

I would prefer to see the fixups merged into the the commits they were fixing but perhaps you did it that way to make it clearer for review. Either way it isn't a big issue.

I will need another couple of days to review these fully but thanks for the work.

I would prefer to see the fixups merged into the the commits they were fixing but perhaps you did it that way to make it clearer for review. Either way it isn't a big issue.

Yeah, my normal workflow is to address fixes to commits by running git commit --fixup=<...> to try to make it clearer what's changed as part of review fixes, and then once the PR has been approved, I'll do that rebase --autosquash command which automatically squashes the fixups into their marked commits.

I'll just push the updated commits in future.

I will need another couple of days to review these fully but thanks for the work.

Great thanks!

I've rebased off master and added two fixups which adds tests for the two commits.

This may be a stupid question but I'm struggling to understand the "monotonic" condition from the test case:

    // This excel has been manually edited so that the si numbers do not monotonically increase (si
    // 0 swapped with 1)

Where is that happening?

Also, could you check if the following condition is handled before/after the changes and add it as a test case if required.

shared_formula_reversed.xlsx

Sheet1 has a standard shared formula in cell "B1" as "=A1". This is shared down to cell "B5".

  <sheetData>
    <row r="1" spans="1:2" x14ac:dyDescent="0.2">
      <c r="A1">
        <v>1</v>
      </c>
      <c r="B1">
        <f>A1</f>
        <v>1</v>
      </c>
    </row>
    <row r="2" spans="1:2" x14ac:dyDescent="0.2">
      <c r="A2">
        <v>2</v>
      </c>
      <c r="B2">
        <f t="shared" ref="B2:B5" si="0">A2</f>
        <v>2</v>
      </c>
    </row>
    <row r="3" spans="1:2" x14ac:dyDescent="0.2">
      <c r="A3">
        <v>3</v>
      </c>
      <c r="B3">
        <f t="shared" si="0"/>
        <v>3</v>
      </c>
    </row>
    <row r="4" spans="1:2" x14ac:dyDescent="0.2">
      <c r="A4">
        <v>4</v>
      </c>
      <c r="B4">
        <f t="shared" si="0"/>
        <v>4</v>
      </c>
    </row>
    <row r="5" spans="1:2" x14ac:dyDescent="0.2">
      <c r="A5">
        <v>5</v>
      </c>
      <c r="B5">
        <f t="shared" si="0"/>
        <v>5</v>
      </c>
    </row>
  </sheetData>

Sheet2 has a shared formula in cell "B5" as "=A5". This is shared "up" to cell "B1".

 <sheetData>
    <row r="1" spans="1:2" x14ac:dyDescent="0.2">
      <c r="A1">
        <v>1</v>
      </c>
      <c r="B1">
        <f t="shared" ref="B1:B4" si="0">A1</f>
        <v>1</v>
      </c>
    </row>
    <row r="2" spans="1:2" x14ac:dyDescent="0.2">
      <c r="A2">
        <v>2</v>
      </c>
      <c r="B2">
        <f t="shared" si="0"/>
        <v>2</v>
      </c>
    </row>
    <row r="3" spans="1:2" x14ac:dyDescent="0.2">
      <c r="A3">
        <v>3</v>
      </c>
      <c r="B3">
        <f t="shared" si="0"/>
        <v>3</v>
      </c>
    </row>
    <row r="4" spans="1:2" x14ac:dyDescent="0.2">
      <c r="A4">
        <v>4</v>
      </c>
      <c r="B4">
        <f t="shared" si="0"/>
        <v>4</v>
      </c>
    </row>
    <row r="5" spans="1:2" x14ac:dyDescent="0.2">
      <c r="A5">
        <v>5</v>
      </c>
      <c r="B5">
        <f>A5</f>
        <v>5</v>
      </c>
    </row>
  </sheetData>

Sheet2 has a shared formula in cell "B5" as "=A5". This is shared "up" to cell "B1".

Just to confirm the shared formula is still "above/before" the range it "shares" to (as is the case in the XML you gave)?
I assume this is how excel generates the XML when the users drags up?

I've added it as a test case and it seems to pass fine.

This may be a stupid question but I'm struggling to understand the "monotonic" condition from the test case:

So essentially I had a normal Excel with 3 formulas and they were given si numbers 0, 1 and 2. I then manually swapped everywhere I saw si=0 referenced with 1 and visa-versa. In the old code the si numbers were required to only ever increase in the order that they were parsed. If you had an si number less than the current max, it would be pushed to the end and would have the wrong index breaking any cells that reference it (they would probably just point to a None entry). After that commit it's no-longer the case.

I have no idea if this happens in practise but I noticed the code when I was fixing the other issue, and it seemed like an easy change to cover some potential edge cases. I did take a look at the spec and I couldn't find anything one way or the other so it seems like it's not guaranteed that the si number only every increases throughout a sheet.

I then manually swapped everywhere I saw si=0 referenced with 1 and visa-versa.

So this is the bit I'm missing. I expected to see something like that in the test file but the si reference still seem to be in order of 0, 1, 2. Here is the relevant xml from the test file:

  <sheetData>
    <row r="1" spans="1:3" x14ac:dyDescent="0.2">
      <c r="A1">
        <v>1</v>
      </c>
      <c r="B1">
        <v>2</v>
      </c>
      <c r="C1">
        <v>3</v>
      </c>
    </row>
    <row r="2" spans="1:3" x14ac:dyDescent="0.2">
      <c r="A2">
        <f>A1+1</f>
        <v>2</v>
      </c>
      <c r="B2">
        <f>B1+2</f>
        <v>4</v>
      </c>
      <c r="C2">
        <f>C1+3</f>
        <v>6</v>
      </c>
    </row>
    <row r="3" spans="1:3" x14ac:dyDescent="0.2">
      <c r="A3">
        <f t="shared" ref="A3:A6" si="0">A2+1</f>
        <v>3</v>
      </c>
      <c r="B3">
        <f t="shared" ref="B3:B6" si="1">B2+2</f>
        <v>6</v>
      </c>
      <c r="C3">
        <f t="shared" ref="C3:C6" si="2">C2+3</f>
        <v>9</v>
      </c>
    </row>
    <row r="4" spans="1:3" x14ac:dyDescent="0.2">
      <c r="A4">
        <f t="shared" si="0"/>
        <v>4</v>
      </c>
      <c r="B4">
        <f t="shared" si="1"/>
        <v>8</v>
      </c>
      <c r="C4">
        <f t="shared" si="2"/>
        <v>12</v>
      </c>
    </row>
    <row r="5" spans="1:3" x14ac:dyDescent="0.2">
      <c r="A5">
        <f t="shared" si="0"/>
        <v>5</v>
      </c>
      <c r="B5">
        <f t="shared" si="1"/>
        <v>10</v>
      </c>
      <c r="C5">
        <f t="shared" si="2"/>
        <v>15</v>
      </c>
    </row>
    <row r="6" spans="1:3" x14ac:dyDescent="0.2">
      <c r="A6">
        <f t="shared" si="0"/>
        <v>6</v>
      </c>
      <c r="B6">
        <f t="shared" si="1"/>
        <v>12</v>
      </c>
      <c r="C6">
        <f t="shared" si="2"/>
        <v>18</v>
      </c>
    </row>
  </sheetData>

Just to confirm the shared formula is still "above/before" the range it "shares" to (as is the case in the XML you gave)?

Yes.

I've added it as a test case and it seems to pass fine.

Great. Thanks.

So this is the bit I'm missing. I expected to see something like that in the test file but the si reference still seem to be in order of 0, 1, 2. Here is the relevant xml from the test file:

That's odd that's not what I see:

$ unzip non_monotonic_si.xlsx -d non_monotonic_si

$ cat non_monotonic_si/xl/worksheets/sheet1.xml | xmlstarlet fo
<?xml version="1.0"?>
<worksheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main" xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" xmlns:r="http://schemas.openxmlformats.org/officeDocument/2006/relationships" xmlns:x14ac="http://schemas.microsoft.com/office/spreadsheetml/2009/9/ac" xmlns:xr="http://schemas.microsoft.com/office/spreadsheetml/2014/revision" xmlns:xr2="http://schemas.microsoft.com/office/spreadsheetml/2015/revision2" xmlns:xr3="http://schemas.microsoft.com/office/spreadsheetml/2016/revision3" xr:uid="{2045C88D-1A8E-3548-9FEB-4D2336B4591F}" mc:Ignorable="x14ac xr xr2 xr3">
  <dimension ref="A1:C6"/>
  <sheetViews>
    <sheetView tabSelected="1" workbookViewId="0">
      <selection activeCell="B5" sqref="B5"/>
    </sheetView>
  </sheetViews>
  <sheetFormatPr baseColWidth="10" defaultRowHeight="16" x14ac:dyDescent="0.2"/>
  <sheetData>
    <row r="1" spans="1:3" x14ac:dyDescent="0.2">
      <c r="A1">
        <v>1</v>
      </c>
      <c r="B1">
        <v>2</v>
      </c>
      <c r="C1">
        <v>3</v>
      </c>
    </row>
    <row r="2" spans="1:3" x14ac:dyDescent="0.2">
      <c r="A2">
        <f>A1+1</f>
        <v>2</v>
      </c>
      <c r="B2">
        <f>B1+2</f>
        <v>4</v>
      </c>
      <c r="C2">
        <f>C1+3</f>
        <v>6</v>
      </c>
    </row>
    <row r="3" spans="1:3" x14ac:dyDescent="0.2">
      <c r="A3">
        <f ref="A3:A7" si="1" t="shared">A2+1</f>
        <v>3</v>
      </c>
      <c r="B3">
        <f ref="B3:B6" si="0" t="shared">B2+2</f>
        <v>6</v>
      </c>
      <c r="C3">
        <f ref="C3:C6" si="2" t="shared">C2+3</f>
        <v>9</v>
      </c>
    </row>
    <row r="4" spans="1:3" x14ac:dyDescent="0.2">
      <c r="A4">
        <f si="1" t="shared"/>
        <v>4</v>
      </c>
      <c r="B4">
        <f si="0" t="shared"/>
        <v>8</v>
      </c>
      <c r="C4">
        <f si="2" t="shared"/>
        <v>12</v>
      </c>
    </row>
    <row r="5" spans="1:3" x14ac:dyDescent="0.2">
      <c r="A5">
        <f si="1" t="shared"/>
        <v>5</v>
      </c>
      <c r="B5">
        <f si="0" t="shared"/>
        <v>10</v>
      </c>
      <c r="C5">
        <f si="2" t="shared"/>
        <v>15</v>
      </c>
    </row>
    <row r="6" spans="1:3" x14ac:dyDescent="0.2">
      <c r="A6">
        <f si="1" t="shared"/>
        <v>6</v>
      </c>
      <c r="B6">
        <f si="0" t="shared"/>
        <v>12</v>
      </c>
      <c r="C6">
        <f si="2" t="shared"/>
        <v>18</v>
      </c>
    </row>
  </sheetData>
  <pageMargins bottom="0.75" footer="0.3" header="0.3" left="0.7" right="0.7" top="0.75"/>
</worksheet>

Ahh I just did a test. I opened the file in Excel and saved and it re-ordered the si numbers. It looks like my version of excel fixes the order on save.

I opened the file in Excel and saved and it re-ordered the si numbers. It looks like my version of excel fixes the order on save.

Apologies. I must have resaved it in that case. Sorry for the noise. The test case make more sense now.

Otherwise this looks good to merge. Can you squash the fixup test cases into the relevant commits and resubmit the PR and I will merge it.

Apologies. I must have resaved it in that case. Sorry for the noise. The test case make more sense now.

No worries :)

Otherwise this looks good to merge. Can you squash the fixup test cases into the relevant commits and resubmit the PR and I will merge it.

Done

once it's merged, I'll rebase my other PR off master and then that should be ready for a review

Merged. Thanks.

I'll rebase my other PR off master and then that should be ready for a review

Sounds good.