-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Revamp dump
#16083
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
base: master
Are you sure you want to change the base?
Revamp dump
#16083
Conversation
local t = type(o) | ||
if not level and t == "userdata" then | ||
-- when userdata (e.g. player) is passed directly, print its metatable: | ||
return "userdata metatable: " .. dump(getmetatable(o)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this undocumented "feature". I think if modders want this, they should do dump(getmetatable(value))
explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was an explicitly requested feature: #6842
re "undocumented": documenting the precise behavior of a pure debugging helper function isn't needed and can only cause us problems in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was an explicitly requested feature: #6842
i wonder whether @HybridDog would still consider this feature a good idea today?
documenting the precise behavior of a pure debugging helper function isn't needed and can only cause us problems in the future.
sure. i'm just trying to argue that i should be somewhat free to remove things like this if i can make a half-decent case that they should be removed.
ultimately i don't mind putting it back in terribly much but i still think it shouldn't work like that. dump({player}, "")
gives you {<userdata>,}
but dump(player, "")
dumps you the (usually annoyingly big) metatable? that's just confusing if you ask me, it breaks the natural recursive nature of dump
and adds a weird special case.
we should probably give our userdata proper __tostring
implementations which identify it. e.g. PlayerRef: singleplayer
or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah if it doesn't work recursively I agree it's not ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if a modder uses dump() on a variable to find out what it is, an output like userdata: 0x7acd0162a3e0
is not very helpful to him/her and showing the metatable is also not ideal. Removing the feature to print the metatable is fine for me.
Adding __tostring
implementations for userdata sounds like a better solution if this is possible.
I think having |
-- Write string-keyed entries | ||
table.sort(keys.string) | ||
for _, k in ipairs(keys.string) do | ||
local v = val[k] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need rawget(val, k)
for the case that val
has a metatable with __index
method?
(analogously below at write_entry(k, tbl[k])
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would hope not: We are just considering keys obtained via pairs
here, which ignores __index
(and there is no __pairs
on 5.1 either). So all these entries should actually exist.
But I guess I might use rawget
anyways so it's more obvious that it ignores the metatable.
local t = type(o) | ||
if not level and t == "userdata" then | ||
-- when userdata (e.g. player) is passed directly, print its metatable: | ||
return "userdata metatable: " .. dump(getmetatable(o)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if a modder uses dump() on a variable to find out what it is, an output like userdata: 0x7acd0162a3e0
is not very helpful to him/her and showing the metatable is also not ideal. Removing the feature to print the metatable is fine for me.
Adding __tostring
implementations for userdata sounds like a better solution if this is possible.
Added a helpful |
I figured it would be nice if the default engine-way of inspecting values was a bit nicer.
Goals:
dump
output should be easy to read.dump
ed strings should produce equivalent values.dump
should be reasonably efficient (linear time in the size of output), but there is not much of a need to squeeze out constant factors.With this PR:
tostring
ed if it doesn't lose precision, otherwise they're formatted exactly.setref
andgetref
, it is even possible to just copy-paste thedump
output and run it as Lua code - see the unit tests.)How to test
Unit tests are included, but feel free to churn more things through
dump
.