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

Fix issue when pulling from cells with some types of formulas is them #23

Conversation

IsakNaslundBh
Copy link
Contributor

@IsakNaslundBh IsakNaslundBh commented Apr 19, 2023

Issues addressed by this PR

Closes #22

As stated in the issue, there are some issues with reading values from some cells that contain some types of formulas.

The code in this PR fixes the issue by falling back to the cached value of the cell for when getting the Value out fails.
I went with raising a warning if this happens, and if closedXML believes the cell needs recalculation. For all cases I have tested so far, the cached value will be correct, but raised the warning to be safe.

Saying that, I was not aware that the adapter actually recomputed the sheet to get the values. Would have assumed that it just got whatever was in the cell, so might be safe to remove the warning.
Happy for any input on this from reviewers.

Test files

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

Changelog

Additional comments

@IsakNaslundBh IsakNaslundBh added the type:bug Error or unexpected behaviour label Apr 19, 2023
@IsakNaslundBh IsakNaslundBh self-assigned this Apr 19, 2023
@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check compliance

Excel_Adapter/Convert/FromExcel/CellContents.cs Outdated Show resolved Hide resolved
Excel_Adapter/Convert/FromExcel/CellContents.cs Outdated Show resolved Hide resolved
Excel_Adapter/Convert/FromExcel/CellContents.cs Outdated Show resolved Hide resolved
Excel_Adapter/CRUD/Read/Read.cs Outdated Show resolved Hide resolved
@FraserGreenroyd FraserGreenroyd force-pushed the Excel_Toolkit-#22-FixIssueReadingSheetsWithSomeTypesOfForumlas branch from 513c578 to 08b75ae Compare May 12, 2023 11:09
Copy link

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

Testing against today's alpha using the test script provided shows these two errors on the canvas:
image

image

After rebasing this PR and compiling this toolkit, the errors change to warnings:

image

Same for both pulls.

This did pull through values whereas the alpha did not, so this PR represents an improvement to the overall workflow. I'm happy for this to be merged and deployed for further testing via alphas.

@FraserGreenroyd
Copy link

@BHoMBot check compliance
@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented May 12, 2023

@FraserGreenroyd to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance
  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

@bhombot-ci
Copy link

bhombot-ci bot commented May 12, 2023

The check code-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@bhombot-ci
Copy link

bhombot-ci bot commented May 12, 2023

The check documentation-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@FraserGreenroyd
Copy link

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented May 12, 2023

@FraserGreenroyd to confirm, the following actions are now queued:

  • check ready-to-merge

@FraserGreenroyd FraserGreenroyd merged commit 97f2eb9 into develop May 12, 2023
@FraserGreenroyd FraserGreenroyd deleted the Excel_Toolkit-#22-FixIssueReadingSheetsWithSomeTypesOfForumlas branch May 12, 2023 11:52
@bhombot-ci bhombot-ci bot mentioned this pull request Jun 12, 2023
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.

Problem reading cells with some types of formulas
2 participants