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 `task_name` to built-in variables #537

Merged
merged 5 commits into from Apr 14, 2017

Conversation

Projects
None yet
2 participants
@komamitsu
Copy link
Member

komamitsu commented Apr 6, 2017

With this feature, users can use a task name in workflow like this:

+task1:
    td>: queries/${task_name.replace(/\+/g, '_')}.sql    # >>> "_myworkflow_task1.sql"
    insert_into: hourly_summary
@komamitsu

This comment has been minimized.

Copy link
Member Author

komamitsu commented Apr 6, 2017

@frsyuki Can you take look at this pull request when you get a chance?

@komamitsu komamitsu requested a review from frsyuki Apr 6, 2017

@frsyuki

This comment has been minimized.

Copy link
Member

frsyuki commented Apr 6, 2017

Very nice 👍
I wondered whether the task name should include the root task name, or not.
In +built_in_variables+get_variables+task_name, +built_in_variables is not precisely task name because it's workflow name. However, internally, workflow name is used as the "root" task name. So it's a sort of task name.
I honestly don't have strong opinion on it. How do you think? Maybe it should include because it's more natural from the internal design's point of view.
Code itself looks very good.

@frsyuki

frsyuki approved these changes Apr 6, 2017

@komamitsu

This comment has been minimized.

Copy link
Member Author

komamitsu commented Apr 10, 2017

Yeah, good point. It's probably better to remove a workflow name from it since users don't know a workflow name is internally included in a task name. I'll modify the pull request later.

komamitsu added some commits Apr 11, 2017

@komamitsu

This comment has been minimized.

Copy link
Member Author

komamitsu commented Apr 11, 2017

Separated the workflow name from a task name like this.

+parent:
  +task0:
      td>: queries/${workflow_name}/${task_name.join('__')}.sql     # >>> "queries/mydig/parent__task0.sql"
      insert_into: hourly_summary

@frsyuki Could you look over the updated one?

@frsyuki

This comment has been minimized.

Copy link
Member

frsyuki commented Apr 14, 2017

@komamitsu yes, LGTM.
Precisely speaking, task name includes +. For example, if full task name is +my_workflow+parent+task1, task name represented as an array would be ["+parent", "+task1"].
But I know that it's not very useful because people need to escape or convert + in many cases.

@frsyuki

This comment has been minimized.

Copy link
Member

frsyuki commented Apr 14, 2017

Oh Mitsu, I recalled one problem. Task name delimiter is not always +. Dynamically generated tasks use ^ as a delimiter (e.g. ^sub, ^failure-alert). So, removing + actually loses some information...

@frsyuki

This comment has been minimized.

Copy link
Member

frsyuki commented Apr 14, 2017

How about just using the first idea? It seems most straightforward way.
Calling task_name.replace(/\W/g, '_')} or tsak_name.split(/\W/).join('__') still works.
I think that using task name as a part of temporary table name is also a common use case but often cases they'll need to escape other symbols such as = because loop / for_each operators includes it. So, documenting use of task_name.replace(/\W/g, '_')} as an example is actually most simple and causes less problems compared to recommending task_name.join('__'), I think.

@komamitsu

This comment has been minimized.

Copy link
Member Author

komamitsu commented Apr 14, 2017

Ah. Good catch!

How about just using the first idea? It seems most straightforward way.

👍

komamitsu added some commits Apr 14, 2017

Revert "Minor rename"
This reverts commit b0d7db2.

@komamitsu komamitsu merged commit 568f9d6 into master Apr 14, 2017

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@komamitsu komamitsu deleted the runtime-param-task-name branch Apr 14, 2017

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.