Skip to content

Conversation

YoRyan
Copy link
Contributor

@YoRyan YoRyan commented May 5, 2022

Note that Lua 5.0 breaks some of our assumptions about which features are common to all versions - see the deletions in core/. That means the tests also need to be updated.

Copy link
Member

@Perryvw Perryvw left a comment

Choose a reason for hiding this comment

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

I am not in favour of splitting these functions out of the core directory and into 15 different version specific files, this seems like a nightmare to maintain. Please keep the core files as they are and reconsider how to do 5.0 specific stuff.

@YoRyan
Copy link
Contributor Author

YoRyan commented May 13, 2022

But the functions I removed from core are simply not available in 5.0, so it doesn't make sense to keep them there. Anything in core must be available to all versions, if I've understood the setup correctly.

You can categorize the new additions into just two domains: "5.0" and "5.1 and above." The only reason I created so many new files was to eliminate as much duplicate code as possible, as is done in the existing declarations. If we were to accept some amount of duplication, that should allow me to simplify things dramatically.

@Perryvw
Copy link
Member

Perryvw commented May 13, 2022

But the functions I removed from core are simply not available in 5.0, so it doesn't make sense to keep them there. Anything in core must be available to all versions, if I've understood the setup correctly.

You can categorize the new additions into just two domains: "5.0" and "5.1 and above." The only reason I created so many new files was to eliminate as much duplicate code as possible, as is done in the existing declarations. If we were to accept some amount of duplication, that should allow me to simplify things dramatically.

I am okay with accepting some duplication, splitting into 5.0 and 5.1+ sounds like a good idea. If possible try keeping the current declarations as they are as much as possible.

@YoRyan
Copy link
Contributor Author

YoRyan commented Jul 25, 2022

This PR should be ready for review again. How does this look? I've moved the declarations for 5.0 into entirely separate files.

@Perryvw Perryvw merged commit b0f29a9 into TypeScriptToLua:master Aug 10, 2022
@YoRyan YoRyan deleted the lua5.0 branch August 10, 2022 18:31
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.

3 participants