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

Refactor task definition #75

Open
tiagofilipe12 opened this Issue Aug 23, 2017 · 3 comments

Comments

2 participants
@tiagofilipe12
Copy link
Member

tiagofilipe12 commented Aug 23, 2017

Current implementation on task definition by the user is somehow javascript dependent.
Example:

const gunzipIt = task({
    input: '*_genomic.fna.gz',
    output: '*.fa',
    params: { output: 'uncompressed.fa' }
  }, ({ params, input}) => `gunzip -c ${input} > ${params.output}`
)

It would be nice to abstract the user from this object, function task shape, to something more user-friendly. For example:

const gunzipIt = { task:
  { input:  '*_genomic.fna.gz',
    output: 'uncompressed.fa',
    function: '`gunzip -c ${input} > ${params.output}`'
  }
}

Notice that I removed params.output, because that can be tricky for the user to understand, and instead this check of the output pattern could be done by the api.
This could be integrated with #74 .

@thejmazz

This comment has been minimized.

Copy link
Member

thejmazz commented Aug 24, 2017

Why

'`gunzip -c ${input} > ${params.output}`'

instead of

`gunzip -c ${input} > ${params.output}`

?

Except then input and params will be undefined: I think we will always still need something with

func: function(resolvedProps) {
  return `gunzip -c ${resolvedProps.input} > ${params.output}`
}

and then, the change is basically just moving the operationCreator from the second param to a property of the props object. Which I am OK with as a change -

const gunzipIt = {
  input:  '*_genomic.fna.gz',
  output: 'uncompressed.fa',
  operation: ({ params, input}) => `gunzip -c ${input} > ${params.output}`
}

(maybe don't need to nest under task? - except for orchestrators?)

One benefit of having the task function is that you still then get an invocableTask which can be used to manually orchestrate tasks or anything.

Either way - I think this is less priority than #74 and probably mostly a style change, unless #74 brings up changes that require changing it here as well.

It feels like to me this simply just changes (propsObject, operationCreator) into (propsObjectWithOperationCreator) since the task function is still needed to parse the definition into something that runs a task lifecycle - invocableTask

@tiagofilipe12

This comment has been minimized.

Copy link
Member Author

tiagofilipe12 commented Aug 24, 2017

I said:

'`gunzip -c ${input} > ${params.output}`'

because internally maybe it can be converted into this:

func: function(resolvedProps) {
  return `gunzip -c ${resolvedProps.input} > ${params.output}`
}

but your suggestion is also nice, where you replace (propsObject, operationCreator) by (propsObjectWithOperationCreator).

And yes, this is just styling to make it easier for users outside the scope of JS to use the tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.