search

joerdav / xc

Markdown defined task runner.
https://xcfile.dev/
MIT License
1.04k stars 25 forks source link

Add option to run dependencies asynchronously #106

Closed stephenafamo closed 7 months ago

stephenafamo commented 7 months ago

Fixes #105

joerdav commented 7 months ago

This looks great! Thanks for the contribution. Would you kindly add some documentation on this feature before I accept the PR https://github.com/joerdav/xc/tree/main/doc/content/task-syntax

Thanks!

stephenafamo commented 7 months ago

Documented 👌🏾

joerdav commented 7 months ago

Brilliant, thanks for that.

Now that parrallel tasks are possible do you think we should do something about the STDOUT of the tasks? Since they may get mangled now.

And a question on runDepsAsync currently we're using an errgroup, so it'll exit as soon as there is an error, do you think it would make more sense to await the other deps to conclude before ending?

stephenafamo commented 7 months ago

Now that parrallel tasks are possible do you think we should do something about the STDOUT of the tasks? Since they may get mangled now.

Well.... the ideal experience may be to do like docker-compose and prefix each log line with the task name. But that adds a bit more complexity.

And a question on runDepsAsync currently we're using an errgroup, so it'll exit as soon as there is an error, do you think it would make more sense to await the other deps to conclude before ending?

Well, I could see people wanting both ways, but I honestly don't know which would be better. I almost want to suggest have both behaviours as options, but it starts to feel even more complex.

The other idea I had was to allow the user group the async task with brackets. Something like this.

requires: build-assets, [watch-assets, watch-server], cleanup

Which will run the build assets first, then run the watchers async, before the cleanup. But again... more complexity.

joerdav commented 7 months ago

I think we can maybe tackle the logging of async tasks later.

For me I think the sensible default would be for the async tasks to all complete before continuing. I think after that change I would be happy to accept the change provided all the workflows pass!

I believe the grouped behaviour is currently achievable once we merge this branch too (even if it's a bit messy):

## run

requires: build-assets, watch, cleanup

## watch

requires: watch-assets, watch-server

runDeps: async

## watch-server
...

## watch-assets
...

## build-assets
...

## cleanup
...
stephenafamo commented 7 months ago

Made the change to not exit on the first error.

stephenafamo commented 7 months ago

I've added log prefixes to all scripts run by xc. And if the task has dependencies, it will pad the prefixes so it all looks nice.

I understand that this would technically be a breaking change, but I think it is allowed since xc is still in v0

stephenafamo commented 7 months ago

I've also fixed a bug with xc not recognizing attributes on the last line of the file in 5b9ae35

I'm not sure if that's the best solution, so please take a look

joerdav commented 7 months ago

It looks like the lint step failed, are you able to fix these? It looks like there was also a large drop in test coverage, I think some tests around the log file you created should be acceptable.

stephenafamo commented 7 months ago

It looks like the lint step failed, are you able to fix these? It looks like there was also a large drop in test coverage, I think some tests around the log file you created should be acceptable.

Fixed linting and added tests

stephenafamo commented 7 months ago

Can you take another look @joerdav

stephenafamo commented 7 months ago

I've also added an "Interactive" attribute with some documentation of where it can be useful.