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

feat: add override as an option to flatten #1748

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

elocke
Copy link

@elocke elocke commented Aug 9, 2024

I got excited about the new flatten feature in #1704 so this extends that work.

Here's my attempt of what could be extending it to override included & flattened tasks. I have an use case for ci similar to #273 (comment).

It's my first contribution here so let me know what you need.

Taskfile.yml
- build [echo "override"]

Taskfile.included.yml
- build ["echo build"]
- test ["echo test"]

> task build
      "override"

> task test
      "test"

@elocke elocke marked this pull request as ready for review September 4, 2024 21:19
@elocke
Copy link
Author

elocke commented Sep 4, 2024

Marking as ready for review since #1704 has merged

@pd93
Copy link
Member

pd93 commented Sep 7, 2024

I'm concerned that the behavior here is confusing. The Taskfile might look like this:

includes:
  included:
    taskfile: "./included"
    flatten: true
    override: true

It is not clear to me which file is taking priority. I would suggest from this syntax that since the override property is specified alongside the included taskfile that the included file would override the parent, but looking at your code/tests, the opposite is true.

Maybe something along these lines is clearer:

includes:
  included:
    taskfile: "./included"
    flatten: true
    prefer: "parent"   # or "child"

(there might be better terms other than parent/child)

@vmaerten as the implementer of flattening, do you have any opinions? I personally don't have a use for this feature, so it's hard for me to be objective.

@vmaerten
Copy link
Member

vmaerten commented Sep 9, 2024

@pd93 I agree with you about the confusion but it's not my main concern.

If we consider one root taskfile that includes two taskfiles, both flatten but one override and the other not, like this :

version: '3'

includes:
  one:
    taskfile: ./one.yml
    flatten: true
    override: true
  two:
    taskfile: ./two.yml
    flatten: true

one and two have at least one task in common.

Example :

version: '3'

task:
  build:
    cmd: echo "build"

Depending on which is included first, the behavior won't be the same. And, personally, I think it's an issue.

(This usecase is also valid, with both as override, which one will be taken into account ? One or two ?)

That's why I did not implement this feature at first.

I am not against this feature but I would like to see a real use case for this

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