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 TimerTask #782

Merged
merged 5 commits into from
Jun 22, 2018
Merged

Add TimerTask #782

merged 5 commits into from
Jun 22, 2018

Conversation

davidor
Copy link
Contributor

@davidor davidor commented Jun 22, 2018

This PR adds a new module that allows running periodic tasks that can be canceled.

@davidor davidor requested a review from a team as a code owner June 22, 2018 10:39
local timer_task = TimerTask.new(test_task)
local id = timer_task.id

timer_task = nil
Copy link

Choose a reason for hiding this comment

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

value assigned to variable 'timer_task' is unused

describe('when the task is active', function()
it('runs the task', function()
local timer_task = TimerTask.new(func, { args = args, interval = interval })
local func_stub = stub(timer_task, 'task')
Copy link

Choose a reason for hiding this comment

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

shadowing upvalue 'func_stub' on line 76

Copy link
Contributor

Choose a reason for hiding this comment

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

Codeclimate does not update comments after initial review?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it writes another comment without deleting the old one.


describe(':execute', function()
local func = test_task
local func_stub
Copy link

Choose a reason for hiding this comment

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

unused variable 'func_stub'

-- @tfield ?table args Arguments to the function
-- @tfield ?number interval Interval in seconds (defaults to 60)
function _M.new(task, options)
local options = options or {}
Copy link

Choose a reason for hiding this comment

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

variable 'options' was previously defined as an argument on line 48

describe('when the task is not active', function()
it('does not run the task', function()
local timer_task = TimerTask.new(func, { args = args, interval = interval })
local func_stub = stub(timer_task, 'task')
Copy link

Choose a reason for hiding this comment

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

shadowing upvalue 'func_stub' on line 76

-- With __gc we can make sure that the task will stop scheduling more work
-- after it has been garbage collected.
local proxy = newproxy(true)
local mt = getmetatable(proxy)
Copy link

Choose a reason for hiding this comment

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

shadowing upvalue 'mt' on line 32

@davidor davidor changed the title [WIP] Add TimerTask Add TimerTask Jun 22, 2018
return self
end

local run_periodic, schedule_next, timer_execute
Copy link
Contributor

Choose a reason for hiding this comment

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

That is very weird style. Why not just define each function as local?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. They all depend on each other so they need to be defined this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly.

run_periodic = function(self)
if not _M.task_is_active(self.id) then return end

self.task(unpack(self.args))
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to use 4a5a815 to make this safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can do that after resty.concurrent is merged.

Copy link
Contributor

@mikz mikz left a comment

Choose a reason for hiding this comment

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

👍

@davidor davidor merged commit bad87e3 into master Jun 22, 2018
@davidor davidor deleted the timer-task branch June 22, 2018 13:56
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.

2 participants