Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Making sure the in-cell formulas are versioned as well #292

Merged
merged 1 commit into from
May 20, 2021

Conversation

adecler
Copy link
Member

@adecler adecler commented May 14, 2021

Issues addressed by this PR

Closes #285

While versioning was happening correctly on the loaded BHoM components, it was not applying the versioning on existing formulas stored in the cells (see additional comments if that sentence doesn't make sense to you).

This PR fixes versioning issue by replacing all the old formulas found in any cell with the new ones.

I would have liked to find a solution that would fix the problem on existing Excel files BUT figuring out the formula signature from the json had too many variations depending on the component type. After attempting that solution for a while, I couldn't avoid the duplication of code and the risk of missing edge cases.

So I decided to go for a much simpler solution where I save the formula at the same time I save the component in the hidden sheet. This results in a much simpler and robust code but sadly only fixing the bug for sheets saved after this PR. Excel not being used too much so far, I think this is a reasonable compromise though. The old spreadsheets also only need to be fixed once and saved and will work fine after that.

Test files

https://burohappold.sharepoint.com/:f:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/Excel_Toolkit/%23285-FormulaVersioning?csf=1&web=1&e=T6QLfL

Only Query.Revit.InfoString?by_String will still give a #NAME error because that component was deleted without replacement (as you can see at the bottom of the new versioning report).

Note this file was manufactured by

  • creating it in 4.0,
  • adding the old formulas info in the hidden sheet while the BHoM was deactivated (so no attempt to upgrade the file)

Additional comments

For those that need clarification:

The BHoM components are registered with Excel as new formulas. Once registered, those are treated as any other formula and don't need any support from the BHoM. The registration of the component using withing the need to be registered every time Excel is opened. So we stored the definition of those components in a hidden spreadsheet, load them when opening the file, version them if needed and register them with Excel.

The formulas, are purely handled by Excel and will just work as long as the corresponding component was registered so the BHoM never manipulates them directly. The signature of a component will change once updated though. So the formula signature will change as well. For example:

Old version: =Query.ModelLaundry.IsPlanar?by_IElement1D_Double(A4)
New version: =Query.Spatial.IsPlanar?by_IElement1D_Double(A4)

So the old formula will still search for the old component that doesn't exist anymore. This is where Excel gives us the #NAME error in the cell.

@adecler adecler added the type:bug Error or unexpected behaviour label May 14, 2021
@adecler adecler self-assigned this May 14, 2021
@adecler
Copy link
Member Author

adecler commented May 14, 2021

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented May 14, 2021

@adecler to confirm, the following checks are now queued:

  • code-compliance
  • documentation-compliance
  • project-compliance
  • core
  • null-handling
  • serialisation
  • installer
  • versioning

@adecler
Copy link
Member Author

adecler commented May 14, 2021

@FraserGreenroyd , same as yesterday: when checking project compliance, the bot gets confused between Versioning_oM and the Version keywork in a dll reference.

@adecler
Copy link
Member Author

adecler commented May 14, 2021

@BHoMBot check copyright-compliance

@bhombot-ci
Copy link

bhombot-ci bot commented May 14, 2021

@adecler to confirm, the following checks are now queued:

  • copyright-compliance

@awakeman
Copy link
Member

Curious as to what happened here, #208 added versioning by registering a dummy formula with the old name that'd rewrite itself by parsing the cell's formula to an expression tree, replacing the upgraded methods in the expression tree, and then re-serialising the expression tree back to a formula. Clearly it wasn't working fully or there was a regression, how does this interact with that code? On of the main things the previous solution was supposed to do if it was functioning as intended was reorder the parameters including when the parameter to an upgraded method was another complex expression containing further BHoM formulae, including if they need upgrading themselves.

Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Probably not related, but I'm curious about the versioning report:

image

given that this spreadsheet was authored with 4.0, should the report state it instead of ?.? ?

@adecler
Copy link
Member Author

adecler commented May 18, 2021

given that this spreadsheet was authored with 4.0, should the report state it instead of ?.? ?

@alelom , the version will only be stored in the file once BHoM/BHoM_Engine#2434 is merged.

@alelom alelom self-requested a review May 18, 2021 08:38
Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Satisfied with the answers, tested and approved from my perspective.

@adecler adecler merged commit efaf06d into master May 20, 2021
@adecler adecler deleted the Excel_UI-#285-FormulaVersioning branch May 20, 2021 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error or unexpected behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"#NAME?" error when reopening previously working Spreadsheet
3 participants