fix(outputter) cache all functions, use io.write not print #637
Conversation
|
since this is less well tested this could really use a review. Anyone interested? @ajacksified @DorianGray @kikito @alerque |
| @@ -31,12 +40,14 @@ return function(options) | |||
| message = pretty.write(message) | |||
| end | |||
|
|
|||
| print(failure:format(counter, t.name)) | |||
| print(string_format(failure, counter, t.name)) | |||
There was a problem hiding this comment.
I get what you are doing with al these function replacements, but as I understand the original issue report and PR, the print() function itself also needs to be cached, but I don't see that happening in this PR. Am I missing something? This and a handful of following statements use print() instead of io_write(). Why do they get a pass?
There was a problem hiding this comment.
more broadly; the outputters should cache the functions they use, since I replaced all usage of print, it no longer needs to be cached, since the same applies to other functions it does cache format, gsub, etc.
There was a problem hiding this comment.
Never mind, I missed L9 entirely! That answers my concern.
| local io_write = io.write | ||
| local io_flush = io.flush | ||
|
|
||
| local function print(msg) |
There was a problem hiding this comment.
It's kind of confusing that methods are cached one way and used with local names (string.format()→string_format()) while global functions are not (print()→print()). Maybe this should be prefixed as a local function _print() and used that way throughout? (also _type etc.)
There was a problem hiding this comment.
If you don't think this is a good idea I'll be fine with that too. It wouldn't be my coding style preference but it gets the job done. The rest of the PR looks okay so if you decide to dismiss this rename suggestion I'll just approve instead.
There was a problem hiding this comment.
I get the point but never seen anything prefixing them with _ ? so I'd rather dismiss this one indeed.
There was a problem hiding this comment.
I see them all the time … but maybe it's mostly in my own code. And it might be a hold-over from Haskell and Rust where it is a compiler-enforced naming scheme for shadowed scope or unused values. Also note luacheck also processes _ as a way to discard a value so there is some precedent for scope delineation. And since you can't use primes like in Haskell...
In any case if you don't like it no biggy.
There was a problem hiding this comment.
the single "_" to ignore something is a very common Lua pattern
fixes #632
closes #633