Skip to content

Conversation

@mununki
Copy link
Member

@mununki mununki commented Dec 12, 2022

This PR fixes #5899

@mununki
Copy link
Member Author

mununki commented Dec 12, 2022

@cknitt Could you let me know what branch this PR goes to? Both 10.1_release in the syntax repo and master like the previous PR?

@cknitt
Copy link
Member

cknitt commented Dec 12, 2022

I would say both 10.1_release and master, but the change is in jscomp/ml which is not part of the syntax, so no need to touch the syntax repo.

@mununki
Copy link
Member Author

mununki commented Dec 12, 2022

Got it! I'll make another PR that goes to 10.1_release.
How about changelog? Adding log to 10.1?

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

This looks great!

@mununki
Copy link
Member Author

mununki commented Dec 12, 2022

@cristianoc Is the support for the empty inlined record type also needed? If it is needed, then I think all we need to remove https://github.com/rescript-lang/rescript-compiler/blob/master/res_syntax/src/res_core.ml#L4657:L4664

type inlinedOptional3 = V0({}) // empty inlined record let ir6 = V0({})
@cristianoc
Copy link
Collaborator

@cristianoc Is the support for the empty inlined record type also needed? If it is needed, then I think all we need to remove https://github.com/rescript-lang/rescript-compiler/blob/master/res_syntax/src/res_core.ml#L4657:L4664

type inlinedOptional3 = V0({}) // empty inlined record let ir6 = V0({})

Can't imagine why one would want that. Do you think there's a possible use case?

@mununki
Copy link
Member Author

mununki commented Dec 12, 2022

Can't imagine why one would want that. Do you think there's a possible use case?

Neither do I. I just want to ask your idea if it is needed.

@cknitt
Copy link
Member

cknitt commented Dec 12, 2022

How about changelog? Adding log to 10.1?

Changelog entry should go to version 10.1.1 in both branches.

@mununki
Copy link
Member Author

mununki commented Dec 12, 2022

How about changelog? Adding log to 10.1?

Changelog entry should go to version 10.1.1 in both branches.

Updated: eb3ff55

@mununki mununki merged commit 6a58feb into master Dec 12, 2022
@mununki mununki deleted the fix-inlined-empty-record branch December 12, 2022 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants