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

Allow standard functions to handle more null values as input #220

Merged

Conversation

lorenzosinisi
Copy link
Contributor

Hi @smn,

I've been testing some of the functions in the standard implementation against their Excel counterparts and noticed some inconsistencies. In certain cases, where the Excel formulas would return nil or an empty string, our implementation either fails or produces different results.

I'm thinking of suggesting improvements to the standard library that could address these issues, while also potentially resolving some other open PRs, like PR 218. Combining this with PR 219 would significantly enhance the robustness of the library.

I’m considering making these changes without breaking backward compatibility, in case anyone relies on the current behavior that returns errors. I'd love to hear your thoughts on this. I hope this approach works for you!

Copy link
Contributor

github-actions bot commented Aug 29, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@lorenzosinisi
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@lorenzosinisi
Copy link
Contributor Author

recheck

@lorenzosinisi
Copy link
Contributor Author

lorenzosinisi commented Sep 3, 2024

Hi @smn what do you think of this one? My only concern is that if anyone is relying on errors coming from the old version of the library they might be surprised so maybe it needs a greater version increase to signal potential breaking changes

Alternatively another thing we could do is to create another library that implements the standard lib in this way and so matches more 1:1 Excel. So that the standard would offer what it currently does, and if one wants can use something like "expression_excel" that comes with a package of custom callbacks which overrides the standard lib in the way suggested in those changes

@smn
Copy link
Contributor

smn commented Sep 26, 2024

@lorenzosinisi this is great, thank you, it adds much needed type checking & resilience to nil values! I think the defaults you've implemented are sane and shouldn't be breaking changes. The failures we were causing earlier are worse, this is an improvement.

@smn smn merged commit b350dad into turnhub:develop Sep 26, 2024
1 of 2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants