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 handling of scenario when attribute name is the same as function name (e.g. "mean(mean)") #911

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

pjanik
Copy link
Member

@pjanik pjanik commented Sep 29, 2023

https://www.pivotaltracker.com/story/show/186152786

mean(mean) is a valid formula, but it used to crash in V3. I realized this would be a problem when I added the isNonFunctionSymbolNode helper as part of the previous PR. This is all due to how MathJS parses expressions (function node + separate function name node).

I also renamed canonicalizeExpression to displayToCanonical, so it's the inverse of canonicalToDisplay.

Additionally, I've added quite a few unit tests that also cover the scenario that caused a real-life bug discussed above and in the PT story.

@pjanik pjanik requested a review from kswenson September 29, 2023 21:18
@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (e8e46cb) 80.71% compared to head (309ef87) 81.22%.

Additional details and impacted files
@@                       Coverage Diff                        @@
##           186144182-canonicalToDisplay     #911      +/-   ##
================================================================
+ Coverage                         80.71%   81.22%   +0.50%     
================================================================
  Files                               305      305              
  Lines                             12587    12587              
  Branches                           3417     3419       +2     
================================================================
+ Hits                              10160    10224      +64     
+ Misses                             2300     2242      -58     
+ Partials                            127      121       -6     
Flag Coverage Δ
cypress 65.88% <14.28%> (+0.48%) ⬆️
jest 60.90% <50.00%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
v3/src/models/data/formula-utils.ts 53.23% <75.00%> (+7.91%) ⬆️
v3/src/models/data/formula.ts 75.75% <50.00%> (ø)
v3/src/models/data/formula-manager.ts 17.77% <0.00%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@kswenson kswenson left a comment

Choose a reason for hiding this comment

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

👍 Looks good

Base automatically changed from 186144182-canonicalToDisplay to main October 3, 2023 21:38
@pjanik pjanik merged commit a407960 into main Oct 3, 2023
6 of 7 checks passed
@pjanik pjanik deleted the 186152786-fn-name-attr-name branch October 3, 2023 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants