Skip to content

Better slash commands, and console printing#4

Open
lilyball wants to merge 3 commits intowildstarnasa:masterfrom
lilyball:slash_lua_dump
Open

Better slash commands, and console printing#4
lilyball wants to merge 3 commits intowildstarnasa:masterfrom
lilyball:slash_lua_dump

Conversation

@lilyball
Copy link

Improve the slash commands, so /lua will evaluate expressions, and /dump will inspect them.

Update the console support to define a print() for expressions to print to the console (only while evaluating code in the console), and make inspect work like /dump.

lilyball added 2 commits June 10, 2014 17:52
Change /lua to evaluate and print an expression. With no args, it
toggles the console.

Add /dump to evaluate and inspect an expression. Currently depth-limited
to 1.

Remove the KeyDown events from the console, as they were causing e.g. '
to unexpectedly print.
Define a print() function for the duration of execution of code in the
console.

Change = to act as though print() were used, including handling multiple
return values.

Change inspect to behave similarly to /dump.

Update /dump to print metatables for userdata.
@draftomatic
Copy link
Member

Silly me, I posted from the wrong github account. The message again:

I might actually pop in and take a look at these push requests. There are a couple of issues I've noticed just from skimming.

Copy link
Author

Choose a reason for hiding this comment

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

Ugh, sorry about the whitespace changes. Houston is not a very good editor (granted, I'm thrilled it exists at all, but its text editing behavior leaves a lot to be desired). I can clean this up if you want.

@draftomatic
Copy link
Member

I mean, just quickly off the top -

  1. Start method names with a capital letter.
  2. What's with the nested handleResults functions everywhere?

@lilyball
Copy link
Author

Start method names with a capital letter.

They're local functions. All the methods start with a capital letter.

What's with the nested handleResults functions everywhere?

Because it's a PITA to handle multiple results from a generic function. You could use { f() } but that doesn't handle nils properly. The only real solution is to have the pcall() be the tail expression in an argument list for another function, because that way you get a ... that you can do things with (e.g. use select() to get proper info about result count and result values, in a manner that works with nil).

The alternative is to write a single helper function that looks like

function collectResults(...)
    local t = { ... }
    t.n = select('#', ...)
    return t
end

and that way you can then work with a table representation of the results that actually contains the count as well, but that's not really any better (especially because you can't e.g. iterate over the table with ipairs() and expect nil values to work).

@draftomatic
Copy link
Member

Take a look at drafto_LuaUtils.lua. There is a Pack(...) method already.

@lilyball
Copy link
Author

I see. I had not looked at drafto_LuaUtils.lua. And you think stuffing it into a table like that is better than the nested function approach?

@LSAnthony
Copy link

The nested functions struck me as quite strange. So yes, I think packing into a table is better ;)

EDIT: Wrong github account again, darnit. This is draftomatic >.<

@lilyball
Copy link
Author

Nested functions aren't really that unusual, but I get that you just may not be used to that style. As it's your project, I will go ahead and update it to use Pack() instead.

Instead of using a nested varargs function, use LuaUtils:Pack() to
produce a table for the varargs.
@lilyball
Copy link
Author

@draftomatic Updated to use LuaUtils:Pack().

@Sinaloit
Copy link
Contributor

Sorry for the long delay, I was expecting @draftomatic to get back to this but I guess he's busy. @kballard I haven't had much chance to look at this or how it would need to be changed to be compatible with the current version. As a note I won't be able to respond until 9/2/14 as I'm travelling until then.

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.

4 participants