-
-
Notifications
You must be signed in to change notification settings - Fork 641
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
feat: add option to declare an included Taskfile as flatten #1704
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please accept my humble code review
fix return
Thanks a lot @ccoVeille for your review. I've done all fixes in one commit |
You are welcome |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @vmaerten, thanks for working on this!
I noticed a bug: dir:
is not being respected on includes:
when flatten: true
is given.
version: '3'
tasks:
pwd:
desc: Print working directory
cmds:
- pwd
includes:
included:
taskfile: ./TaskfileIncluded.yml
dir: ./website
flatten: true
With flatten: true
, it prints the root directory instead of the specified one (dir: ./website
in my example).
Also @pd93 mentioned here (#273 (comment)) that he has refactors in mind that could affect this work. Worth getting an opinion before merging.
Thank @andreynering for the review. I've forgot the AdvancedImport's part |
Awesome to see this feature in a PR, I've been eager to build something with this. An usecase that I would benefit from, would be if I could specify |
I agree but for now, I would keep this feature as simple as possible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also @pd93 mentioned here (#273 (comment)) that he has refactors in mind that could affect this work. Worth getting an opinion before merging.
I have some WIP code that removes the merging altogether. This feature will need to be reworked into that at some point. However, I've not had the time to work on it recently and I don't see any reason to delay this functionality given its popularity.
The PR seems well thought out and reimplementing it if merging is removed should be simple enough.
1 minor comment, but otherwise happy. Thanks for working on this @vmaerten! 🚀
} | ||
|
||
func (err *TaskNameFlattenConflictError) Error() string { | ||
return fmt.Sprintf(`task: Found multiple tasks (%s) included by "%s""`, err.TaskName, err.Include) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is also returned when a task in a child taskfile conflicts with one in a parent taskfile (as opposed to two children taskfiles). We could improve the message to include those cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think about CodeTaskNameConflict
or another one ?
At first I used CodeTaskNameConflict
but I found more readable to distinct both. That being said, it would make sense, so I'll give a try to adjust the message
It's one of the most upvoted feature ! So I gave it a try
Closes : #273