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

Provide high-level interface to run go-sarah #72

Closed
oklahomer opened this issue May 2, 2019 · 0 comments
Closed

Provide high-level interface to run go-sarah #72

oklahomer opened this issue May 2, 2019 · 0 comments
Labels
breaking change v2 https://github.com/oklahomer/go-sarah/issues/68

Comments

@oklahomer
Copy link
Owner

Two steps -- sarah.NewRunner() and Runner.Run() -- are required to initialize and execute sarah.Runner with the current implementation, which introduces some potential error-proneness and cumbersomeness.

First, because initialization of Runner and execution is separated, there is a risk that developers may call Run() method multiple times without the care of Runner's state. Sarah's worker solves this problem with a simple approach as below:

// Run creates as many child workers as specified by *Config and start them.
// When Run completes, Worker is returned so jobs can be enqueued.
// Multiple calls to Run() creates multiple Worker with separate context, queue and child workers.
func Run(ctx context.Context, config *Config, options ...WorkerOption) (Worker, error) {

Run() does not belong to a Worker instance, but belong to workers package as a global function. When Run() is called, this package initializes a new worker instance, runs this and returns this as a workers.Worker interface. This simplifies initialization process, and furthermore, this prohibits developers from calling execution method of an already running worker. sarah.Runner can also benefit from employing the same approach to lower the potential error-proneness and cumbersomeness.

One more modification is to stash the RunnerOptions in a sarah's package scope variable and refer them on execution. Currently, developers must pass options to sarah.NewRunner() explicitly, so the code before sarah.Runner initialization tends to become relatively longer. The use of sarah.RunnerOptions to stash a group of sarah.RunnerOption helped this, but there still was room to improve:

go-sarah/runner.go

Lines 106 to 140 in a7408dc

// RunnerOptions stashes group of RunnerOption for later use with NewRunner().
//
// On typical setup, especially when a process consists of multiple Bots and Commands, each construction step requires more lines of codes.
// Each step ends with creating new RunnerOption instance to be fed to NewRunner(), but as code gets longer it gets harder to keep track of each RunnerOption.
// In that case RunnerOptions becomes a handy helper to temporary stash RunnerOption.
//
// options := NewRunnerOptions()
//
// // 5-10 lines of codes to configure Slack bot.
// slackBot, _ := sarah.NewBot(slack.NewAdapter(slackConfig), sarah.BotWithStorage(storage))
// options.Append(sarah.WithBot(slackBot))
//
// // Here comes other 5-10 codes to configure another bot.
// myBot, _ := NewMyBot(...)
// optionsAppend(sarah.WithBot(myBot))
//
// // Some more codes to register Commands/ScheduledTasks.
// myTask := customizedTask()
// options.Append(sarah.WithScheduledTask(myTask))
//
// // Finally feed stashed options to NewRunner at once
// runner, _ := NewRunner(sarah.NewConfig(), options.Arg())
// runner.Run(ctx)
type RunnerOptions []RunnerOption
// NewRunnerOptions creates and returns new RunnerOptions instance.
func NewRunnerOptions() *RunnerOptions {
return &RunnerOptions{}
}
// Append adds given RunnerOption to internal stash.
// When more than two RunnerOption instances are stashed, they are executed in the order of addition.
func (options *RunnerOptions) Append(opt RunnerOption) {
*options = append(*options, opt)
}

However, this also leads to a new limitation. A single process cannot initialize and run multiple sarah.Runner with different settings because RunnerOptions are going to be stored in a package scope variable and referenced on runner execution. The author thinks this is acceptable because multiple runner executions can simply be achieved by running multiple processes with one runner in each of them.

In exchange for such minor limitation, option registration can be much easier as below:

package hello

func init() {
        // Directly register sarah.CommandProps
	sarah.RegisterCommandProps(
		sarah.NewCommandPropsBuilder().
			BotType(slack.SLACK).
			Identifier("hello").
			InputExample(".hello").
			MatchPattern(regexp.MustCompile(`\.hello`)).
			Func(func(_ context.Context, _ sarah.Input) (*sarah.CommandResponse, error) {
				return slack.NewStringResponse("Hello!"), nil
			}).
			MustBuild(),
	)
}
package main

import (
        // Since sarah.CommandProps is directly registered in hello package, this works by simply importing this.
        _ "hello"
)

func main() {
        config := sarah.NewConfig()
        sarah.Run(context.BackGround(), config)
}
@oklahomer oklahomer added breaking change v2 https://github.com/oklahomer/go-sarah/issues/68 labels May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change v2 https://github.com/oklahomer/go-sarah/issues/68
Projects
None yet
Development

No branches or pull requests

1 participant