Skip to content

Make shared formula cell replacement more greedy#664

Open
OSjoerdWie wants to merge 1 commit into
tafia:masterfrom
OSjoerdWie:shared_formula_names
Open

Make shared formula cell replacement more greedy#664
OSjoerdWie wants to merge 1 commit into
tafia:masterfrom
OSjoerdWie:shared_formula_names

Conversation

@OSjoerdWie

Copy link
Copy Markdown

Fixes the issue described in #663

Note that I made an additional (non issue related) commit due to build failure of the forked repo.

Comment thread examples/read_picture_data.rs Outdated
Comment thread src/xlsx/mod.rs
@jmcnamara

Copy link
Copy Markdown
Collaborator

The core change looks good. I have made some change requests to other parts of the PR.

@louisdewar can you double check this. The change came in with your commit 38ef0e3

Note that I made an additional (non issue related) commit due to build failure of the forked repo.

I'm not sure why that is. It didn't fail on main. I'll look into it. But either way, omit it from this PR.

@jmcnamara jmcnamara self-assigned this Jun 16, 2026
@jmcnamara jmcnamara added next_release needs work for merge The PR needs some rework or clarification. No suitable for merge, yet. labels Jun 16, 2026
@jmcnamara jmcnamara mentioned this pull request Jun 16, 2026
@OSjoerdWie OSjoerdWie force-pushed the shared_formula_names branch from e522a57 to d543306 Compare June 17, 2026 06:29
@jmcnamara

Copy link
Copy Markdown
Collaborator

Could you:

  1. Mark the comments as resolved, if they are resolved.
  2. Fix the formatting issue.: cargo fmt -- --check
  3. Squash the update into the main commit.

…BLE should not become NO2_VARIABLE or NP1_VARIABLE)
@OSjoerdWie OSjoerdWie force-pushed the shared_formula_names branch from d543306 to 65b6999 Compare June 18, 2026 08:01
@louisdewar

louisdewar commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Hi, yes I wasn't thinking about defined names with this.
This fix seems generally good, as far as I can tell the logic is to expand the definition of what is considered to "possibly" be a range to include characters that constitute defined names and then let the defined name get rejected later on when it's seen that it's not really a valid range. Before it would tokenise and think that was a valid range, now it would tokenise <NO1_VARIABLE> and reject it as a range to be "replaced".

I think there's a few issues with some edge cases:

  1. NO1.VARIABLE
  2. \A1

I believe these are all "valid" defined names which I think will get treated as ranges.

I think you can fix by just adding '.' and '\' to the char compare.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs work for merge The PR needs some rework or clarification. No suitable for merge, yet. next_release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants