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

Add Translation for MathF (#18842) #24829

Merged
merged 1 commit into from
Jun 7, 2021
Merged

Conversation

ntovas
Copy link
Contributor

@ntovas ntovas commented May 4, 2021

Fixes #18842

@dnfadmin
Copy link

dnfadmin commented May 4, 2021

CLA assistant check
All CLA requirements met.

@smitpatel
Copy link
Member

  • The reflection is going wrong in those types causing errors because methods are not found.
  • You will need to add a test for each new method translation you are adding.

@bricelam
Copy link
Contributor

bricelam commented May 4, 2021

@smitpatel Should we translate Math.PI to PI()? (might be a bit silly for a constant...)

@smitpatel
Copy link
Member

If it is easy, sure.

@ntovas
Copy link
Contributor Author

ntovas commented May 4, 2021

@smitpatel hey, I couldn't find where are the tests for this functionality.

@bricelam
Copy link
Contributor

bricelam commented May 4, 2021

If it is easy, sure.

Needs a new member translator, but it should be pretty easy. Maybe I'll tack it on when merging this PR... (unless @ntovas wants to give it a go first)

@bricelam
Copy link
Contributor

bricelam commented May 4, 2021

...we should probably file an issue to translate the Cosmos math functions too...

@roji
Copy link
Member

roji commented May 4, 2021

@bricelam there's #24463 waiting for @maumar in case you're in the math mood.

@ntovas
Copy link
Contributor Author

ntovas commented May 4, 2021

@bricelam I would like to give to try it, but I will need a bit more context, specially on member translator

Also I could make the same fix for CosmosDb

@ntovas ntovas force-pushed the AddMathFTranslations branch 3 times, most recently from db4c7d0 to 7c1996f Compare May 8, 2021 12:07
@smitpatel
Copy link
Member

Cosmos tests are failing.

@ntovas
Copy link
Contributor Author

ntovas commented May 12, 2021

Cosmos tests are failing.

I will look at them and will add any changes, most likely on Saturday, if it's ok for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Translate MathF members to SQL functions
6 participants