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

Support localization with updated PowerFx version #276

Conversation

NavneetThekkumpat
Copy link
Contributor

@NavneetThekkumpat NavneetThekkumpat commented Jul 11, 2023

With Power Fx Interpreter version 0.2.6-preview
PowerFxConfig constructor to no longer accept Locale as an input parameter. This immediately breaks Test Engine's support for localization in Test Plans. The PowerFx PR with the changes >> https://github.com/microsoft/Power-Fx/pull/1292/files#diff-9022339f3f8c7c746994da6ac3f95ec05eadaaae5a3eb5ddd7691a226af4a279.

Problem to Solve
Support localization in Test Engine.

Proposal
Supporting 2 current scenarios

  1. Running teststeps with no syntax errors (Run engine.eval once for multiple test steps)
    • Pass locale as a ParseOption within Microsoft.PowerFx.RecalcEngine.Eval() when executing test steps
  2. Running teststeps with syntax errors (Run engine.eval step by step)

Checklist

  • The code change is covered by unit tests. I have added tests that prove my fix is effective or that my feature works
  • I have performed end-to-end test locally.
  • New and existing unit tests pass locally with my changes
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I used clear names for everything
  • I have performed a self-review of my own code

@NavneetThekkumpat NavneetThekkumpat marked this pull request as ready for review July 12, 2023 13:49
@NavneetThekkumpat NavneetThekkumpat requested a review from a team as a code owner July 12, 2023 13:49
Copy link
Contributor

@arpavan arpavan left a comment

Choose a reason for hiding this comment

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

Posted one clarifying question, other than that everything looks good!

@NavneetThekkumpat NavneetThekkumpat merged commit 14fb937 into users/arpavan/upgrade-to-powerfx-0.2.6-preview Jul 12, 2023
1 check passed
@NavneetThekkumpat NavneetThekkumpat deleted the users/nthekkumpat/fix-powerfx-locale branch July 12, 2023 14:43
@NavneetThekkumpat NavneetThekkumpat self-assigned this Jul 18, 2023
arpavan pushed a commit that referenced this pull request Jul 18, 2023
arpavan added a commit that referenced this pull request Jul 19, 2023
* updated sample for calculator/testPlanWithCommaForDecimal.fx.yaml to use a different app

* upgraded PowerFx.Interpreter Microsoft.PowerFx.Interpreter to 0.2.6-preview

* Refactor DateValue field and fix other compile time errors (#275)

* refactored all references of DateValue.Value to DateValue.GetConvertedValue(null)

* changed DateValue.Value to DateValue.GetConvertedValue(null)

* Updated implementation of IUntypedObject

* temporarily removed locale from PowerFxConfig constructor to unblock other fixes

---------

Co-authored-by: Pavan Agnihotri <1485950+arpavan@users.noreply.github.com>

* Support localization with updated PowerFx version (#276)

* supporting locale

* Handling setProperty invalid argument type error when value is number (#278)

* setting numberisfloat config to true

* commenting

* commenting

* refactoring

* refactoring

* updated sample for calculator/testPlanWithCommaForDecimal.fx.yaml

* fixed PowerFxEngineTests after rebasing

---------

Co-authored-by: Pavan Agnihotri <1485950+arpavan@users.noreply.github.com>
Co-authored-by: Navaneeth Thekkumpat <98551644+NavneetThekkumpat@users.noreply.github.com>
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.

3 participants