Skip to content

Add support for more code conversion for migration #154

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

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

seekshiva
Copy link
Contributor

No description provided.

@zth
Copy link
Collaborator

zth commented Oct 10, 2023

This is great, thank you! Don't hesitate to add more if you find.

@zth
Copy link
Collaborator

zth commented Oct 10, 2023

@seekshiva is it ready to be merged?

@seekshiva
Copy link
Contributor Author

@zth This was required to convert one of our internal repos into rescript. If we find more such cases while converting other repos, will raise more PRs in future.

That's it for now.

@seekshiva
Copy link
Contributor Author

seekshiva commented Oct 10, 2023

I also found that Js.Math.random_int() isn't supported. We should either prevent this from getting converted into Math.random_int() or we need to add random_int into the Math module so the converted code works well.

I wasn't sure which direction to go in. If you have suggestion on this, I can implement it in the same PR or in a different one.

@zth
Copy link
Collaborator

zth commented Oct 11, 2023

I also found that Js.Math.random_int() isn't supported. We should either prevent this from getting converted into Math.random_int() or we need to add random_int into the Math module so the converted code works well.

I wasn't sure which direction to go in. If you have suggestion on this, I can implement it in the same PR or in a different one.

Ah right, yes, that sounds useful. Separate PR would be good. There's a Math.Int module where we can put it.

@zth zth merged commit 106a882 into rescript-lang:main Oct 11, 2023
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