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

Test to reproduce #143 #144

Closed
wants to merge 2 commits into from
Closed

Test to reproduce #143 #144

wants to merge 2 commits into from

Conversation

kynx
Copy link

@kynx kynx commented Oct 26, 2023

This PR adds a test to reproduce #143. Not sure if I'm asserting the right thing, but I'll update it if I can figure out a fix.

@kynx
Copy link
Author

kynx commented Oct 26, 2023

Actually I have no idea what should be done here.

Maybe detect if the array contains and keys with an Identifier for a name and then return a Literal() for the whole thing, like It did before 4.1.1? That seems kind of kludgy, but it is what I have to do when generating these arrays. @dg any suggestions?

@kynx
Copy link
Author

kynx commented Oct 29, 2023

@dg I don't know what to do here. That commit is a major BC break for me. Should I revert the whole thing or are there bits that don't break stuff for people?

It would be lovely if this library could actually handle identifiers as array keys. Maybe I could help with that, given a general plan of action for the a next major. But right now I can no longer round trip classes generated with this library. It needs fixing.

@dg
Copy link
Member

dg commented Oct 29, 2023

Probably the best way, given backwards compatibility, is to just return the whole array as Literal, the way it used to work…

@dg dg force-pushed the master branch 2 times, most recently from eb06bf1 to c92f8de Compare October 29, 2023 22:49
@dg dg closed this Oct 29, 2023
@kynx
Copy link
Author

kynx commented Oct 30, 2023

👍 Many thanks for that!

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.

Extract fails on array properties with class constant keys
3 participants