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

fix: templates on task descriptions #1343

Merged
merged 1 commit into from
Oct 7, 2023

Conversation

blackjid
Copy link
Contributor

Issue

When a task description has uses templates, they are not properly compiled when listing the tasks.

This was reported back in #276 and then fixed in #283

I think then there was a regression in f22389a

I don't have much experience in go and in this project context, I've tried my best to find the real issue.

g.Go(func() error {
compiledTask, err := e.FastCompiledTask(taskfile.Call{Task: task.Task})
if err == nil {
task = compiledTask
}
task = compiledTask
tasks[idx] = compiledTask
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if there is any problem by modifying the tasks slice from the go subroutines.. All tests passed on my local machine

Copy link
Member

Choose a reason for hiding this comment

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

We'll never be accessing a map entry from more than one goroutine, so I think this fine.

@@ -458,13 +458,14 @@ func (e *Executor) GetTaskList(filters ...FilterFunc) ([]*taskfile.Task, error)

// Compile the list of tasks
for i := range tasks {
task := tasks[i]
idx := i
task := tasks[idx]
g.Go(func() error {
compiledTask, err := e.FastCompiledTask(taskfile.Call{Task: task.Task})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also think that this err could be just _ and remove the if that follows..

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, this is not needed.

Copy link
Member

@pd93 pd93 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for catching this and finding the cause.

Something something GOEXPERIMENT=loopvar 😅

@andreynering
Copy link
Member

Thanks @blackjid!

@andreynering andreynering merged commit 05755f3 into go-task:main Oct 7, 2023
10 checks passed
andreynering added a commit that referenced this pull request Oct 7, 2023
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.

3 participants