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 table.copy_with_metatables #15754

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

appgurueu
Copy link
Contributor

Implements what I have suggested in #15744.

How to test

There are basic unit tests. Feel free to add more.

@appgurueu appgurueu added the Feature ✨ PRs that add or enhance a feature label Feb 4, 2025
y5nw
y5nw previously approved these changes Feb 4, 2025
Copy link
Contributor

@y5nw y5nw left a comment

Choose a reason for hiding this comment

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

Looks good aside from some minor things.

(Edit: For some reason Github seems to show batched follow-up comments without much context on previous posts.)

builtin/common/misc_helpers.lua Outdated Show resolved Hide resolved
builtin/common/tests/misc_helpers_spec.lua Show resolved Hide resolved
builtin/common/misc_helpers.lua Outdated Show resolved Hide resolved
builtin/common/misc_helpers.lua Outdated Show resolved Hide resolved
@Zughy Zughy added @ Script API One approval ✅ ◻️ Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Feb 4, 2025
@y5nw y5nw linked an issue Feb 4, 2025 that may be closed by this pull request
@Desour

This comment was marked as outdated.

@appgurueu appgurueu removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 5, 2025
doc/lua_api.md Outdated
* returns a deep copy of `table`
* since 5.12:
* `table` can also be non-table value, which will be returned as-is.
* `preserve_metatables`: whether to preserve metatables as they are,
Copy link
Member

Choose a reason for hiding this comment

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

This might need another remark:

Due to implementation details, specifying `preserve_metatables` on engine versions
without `core.features.table_copy_preserve_metatables` is likely to cause errors.

@appgurueu
Copy link
Contributor Author

I think I will turn this into table.copy (old behavior) and table.copy_with_metatables. Saves everyone involved the headache:

  • More readable than a true parameter.
  • No need for a core.features entry; it'll just be nil and cause errors when calling it is attempted when unavailable, as it should be.
  • No concerns about the true being passed as the seen table.

@appgurueu appgurueu added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 9, 2025
@appgurueu appgurueu removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 16, 2025
@appgurueu appgurueu changed the title Allow table.copy to preserve metatables Add table.copy_with_metatables Feb 16, 2025
@y5nw y5nw dismissed their stale review February 16, 2025 16:16

Re-reviewing

@@ -4142,6 +4142,11 @@ Helper functions
* returns time with microsecond precision. May not return wall time.
* `table.copy(table)`: returns a table
* returns a deep copy of `table`
* strips metatables, but this may change in the future
Copy link
Contributor

@y5nw y5nw Feb 16, 2025

Choose a reason for hiding this comment

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

IMO we don't need the metatable-related behavior to change in the future since we have table.copy_with_metatable() as a separate function now.

@sfan5
Copy link
Collaborator

sfan5 commented Feb 16, 2025

shorter name suggestion: table.metacopy

@Zughy
Copy link
Contributor

Zughy commented Feb 17, 2025

Wouldn't be better to have a boolean as an extra parameter in the original table.copy?

@y5nw
Copy link
Contributor

y5nw commented Feb 17, 2025

Wouldn't be better to have a boolean as an extra parameter in the original table.copy?

The issue is that table.copy from the current master takes an undocumented second argument seen, which - as a table (or nil/false, in which case the argument is then initialized with an empty table) - keeps tracks of references that table.copy has seen to handle references (in particular, recursive data structures) correctly. This prevents the use of a preserve_metatables flag as the second argument since it would break compatibility on older versions, where table.copy(t, true) causes an error: #15754 (comment)

(Adding the option as a third parameter would be doable, but it is IMO annoying since the second parameter would almost alwasy be {} or nil with some very few exceptions.)

@nauta-turbidus
Copy link
Contributor

I'd support metacopy. It makes sense and if anyone is confused, docs are there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

table.copy removes metatables
7 participants