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 Type Alias and Fix Record Type Translation in ts2mls #143

Merged
merged 19 commits into from
Oct 10, 2022

Conversation

NeilKleistGao
Copy link
Member

Related Issue: #134

@NeilKleistGao
Copy link
Member Author

I think we may separate namespace and alias from class and trait parsing(case (KEYWORD(k @ ("class" | "trait" | "type" | "namespace")), l0) :: c), because their syntax should be different. e.g. a namesapce should not have a parent namespace. 🤔

@LPTK
Copy link
Contributor

LPTK commented Oct 4, 2022

I find it's better to be very lax and do semantic validation afterwards, because parsing errors are usually worse than semantic errors.

@NeilKleistGao
Copy link
Member Author

OK! And to keep NewParser.scala unchanged, I genetate type alias like this type Option: (None) | (Some<A>). Is this syntax acceptable?

@LPTK
Copy link
Contributor

LPTK commented Oct 4, 2022

Not really no, we need = here. It's fine to extend NewParser.scala for that. But it should check that = is used for aliases and : for classes.

@NeilKleistGao NeilKleistGao changed the title Add Type Alias in ts2mls Add Type Alias and Fix Record Type Translation in ts2mls Oct 4, 2022
@NeilKleistGao
Copy link
Member Author

Also fixed a typo getTypeParametes.

@NeilKleistGao NeilKleistGao marked this pull request as ready for review October 5, 2022 02:15
@NeilKleistGao NeilKleistGao requested a review from LPTK October 5, 2022 02:23
Copy link
Contributor

@LPTK LPTK left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM!

shared/src/main/scala/mlscript/helpers.scala Outdated Show resolved Hide resolved
ts2mls/js/src/main/scala/ts2mls/JSWriter.scala Outdated Show resolved Hide resolved
@LPTK
Copy link
Contributor

LPTK commented Oct 7, 2022

After addressing the comments don't forget to use "squash and merge" and clean up the commit message.

@LPTK LPTK merged commit 09d0184 into hkust-taco:mlscript Oct 10, 2022
@NeilKleistGao NeilKleistGao deleted the tp branch August 30, 2023 02:05
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