Execute a command on current thread

[pocke]

I've added a feature to execute a command on current thread.

Currently, we can execute a command on current thread by the following code.

require 'expeditor'
require 'concurrent/executor/immediate_executor'

service = Expeditor::Service.new executor: Concurrent::ImmediateExecutor.new
command = Expeditor::Command.new(service: service){puts 1; sleep 1; puts 2}

puts 'start'
command.start
puts 'end'

However, it's complexity. So, I've added Command#run method to execute on current thread.
The following code works same as the above.

service = Expeditor::Service.new
command = Expeditor::Command.new(service: service){puts 1; sleep 1; puts 2}

puts 'start'
command.run  # use run instead of start
puts 'end'

Naming of run is inspired by exec.Command of Golang. https://golang.org/pkg/os/exec/#Cmd.Run

[pocke]

TODO: remove unneeded clone in with_fallback method

[pocke]

I've changed the approach of the PR. So, I'll re-create a new PR for the purpose.
I've created a new PR ( #16 ), but I use this( #13 ) instead of the new PR.

[pocke]

Feature

Add Command#run method

I've added Expedior::Command#run method.
This method is similar to the Command#start, but the method is executed on the current thread.

Internal implementation

Initialise futures timing

Expeditor::Command should switch an executor to execute on current thread. So, I've changed timing of initialising futures. Futures are initialised before just start or run.

Add tests

[gfx]

[IMO] The name of run seems difficult to learn because it never implies the running thread.

I prefer more descriptive names, like start_on_current_thread.

[k0kubun]

I prefer more descriptive names, like start_on_current_thread.

I think start implies "start asynchronous execution". Since run is synchronous, at least it should not start with "start".

The name of run seems difficult to learn because it never implies the running thread.

While run does not imply running thread, using current thread for synchronous execution is natural. So I prefer run rather than start_on_current_thread.

[k0kubun]

Oh, probably I'm confused about synchronous/asynchronous. Never mind.

[pocke]

@gfx

The name of run seems difficult to learn because it never implies the running thread.

yes, I think so, too.

I prefer more descriptive names, like start_on_current_thread.

I think start_on_current_thread is too long.
So, #start receives a keyword argument for this feature. and remove run method.
e.g.

command = Expeditor::Command.new{xxx}
command.start(current_thread: true) # execute on current thread

What do you think?

[gfx]

Typing start(current_thread: true) seems much more difficult than start_on_current_thread because my text editor, RubyMine, can suggest instance methods, although the auto-completion does not always work perfectly.

[adorechic]

How about start_with_retry? It should be rename to start_with_retry_on_current_thread ?
WDYT?

[gfx]

@adorechic

Nice suggestion. I think start_with_retry_on_current_thread is not a bad name because it's really clear and auto-completetion friendly, but I know there's someone who hates it.

[pocke]

I'll implement this as a keyword argument.
@gfx thanks for your suggestion.

[adorechic]

I prefer to be more simply @normal_future && @normal_future.executed?

[pocke]

I think it's not good. Because @normal_future should be hidden from library users.
I follow the style guide if it exists.

Note: cookpad/styleguide doesn't have the rule.

[adorechic]

should be hidden from library users.

I agree it, but @normal_future && @normal_future.executed? doesn't returns @normal_future, does it?
Although it might returns nil, nil does not show any information about @normal_future.

[pocke]

I was confused. The implementation doesn't return a future. I'll change it to avoiding !!.

[adorechic]

@pocke don't you support start_with_retry?

[adorechic]

unless seems to be more readable.

[pocke]

@pocke don't you support start_with_retry?

Thanks, I forgot it. I'll implement it.

[adorechic]

current_thread is not a member of retryable_options, so this variable name is a little confusing.
I think you should rename it, for example options.

[pocke]

👍 I think so, too. I'll rename it.

[pocke]

I've fixed the suggestions.
Please review the code.

[adorechic]

should is unnecessary description. it 'executes on current thread' is enough.

[adorechic]

LGTM with a comment

[pocke]

should is unnecessary description. it 'executes on current thread' is enough.

Thanks, I've should from description.