-
Notifications
You must be signed in to change notification settings - Fork 144
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
metro syntax change #572
metro syntax change #572
Conversation
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.
awesome!
a few nits...
lua/metro.lua
Outdated
@@ -14,118 +14,123 @@ Metro.metros = {} | |||
Metro.available = {} | |||
Metro.assigned = {} | |||
|
|||
--- assign | |||
|
|||
--- init/assign | |||
-- "allocate" a metro (assigns unused id) |
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.
"allocate" => initialize?
lua/metro.lua
Outdated
local time = arg_time or 1 | ||
local count = arg_count or -1 | ||
|
||
print("what "..time) |
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.
remove (unless this is intended)?
self.count = -1 | ||
self.callback = nil | ||
self.init_stage = 1 | ||
local m = {} |
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.
while you're at it, consider a table constructor?
m.props = {
id = id,
time = 1,
count = -1,
...
}
lua/metro.lua
Outdated
m.props.id = id | ||
m.props.time = 1 | ||
m.props.count = -1 | ||
m.props.event = nil |
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.
isn't this nil
already (or do we check for the key somewhere?)
m.props.event = nil | ||
m.props.init_stage = 1 | ||
setmetatable(m, Metro) | ||
return m | ||
end | ||
|
||
--- start a metro | ||
-- @param time - (optional) time period between ticks (seconds.) by default, re-use the last period | ||
-- @param count - (optional) number of ticks. infinite by default | ||
-- @param stage - (optional) initial stage number (1-based.) 1 by default | ||
function Metro:start(time, count, stage) |
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.
perhaps consider a similar table approach here? iow:
m:start{time = 0.1, count = 1, stage = 3}
@tehn do you still want to do this? |
holding off for the next breaking-change release, and need to do doc revisions which are not ready. |
re: github.com//issues/570
this is a breaking change:
metro:init()
removed as it was unused (simply resets metro to defaults)metro.alloc
is nowmetro.init
callback
renamedevent
init
ie
and you can still
if you want.
much breakage in dust, however
metro.alloc
->metro.init
m.callback
->m.event
count is -1 by default, need not be specified
time is 1 by default, need not be specified
norns study must be updated (!)
this merge should coincide with dust fix and study fix