-
-
Notifications
You must be signed in to change notification settings - Fork 640
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: specify init filename with --taskfile flag #2018
base: main
Are you sure you want to change the base?
Conversation
previously, it was not possible to specify which filename to use when initializing a new Taskfile as it was hardcoded as "Taskfile.yml". now the --taskfile flag specifies where to write the file to, and the first * contained in it will be replaced by "Taskfile", so `task -it *.yaml` will create a `Taskfile.yaml` file.
* fix Flags header being inside tip admonition * change -t flag's default column and add a description * add Default Filenames section
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.
Thanks for this! Generally supportive of this improvement. Just a couple of discussion points on the implementation/docs.
@@ -16,6 +16,8 @@ import ( | |||
"github.com/go-task/task/v3/internal/sysinfo" | |||
) | |||
|
|||
const DefaultTaskInitFilename = "Taskfile.yml" |
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.
We're in the process of stabilising/reducing the package API surface. I don't think this needs to be here. It's more convenient to keep this with the code that produces the file IMO.
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.
keep this with the code that produces the file
Do you mean near InitTaskfile()
? Or where it is used?
I've defined it as a const above defaultTaskfiles
so it's easier to find in case it ever needs to be changed, instead of mixed in with runtime logic as a literal string.
Also, I didn't define it at init.go
because IIRC I couldn't reference it from task.go
.
I'm open to suggestions on what to do instead.
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 mean near
InitTaskfile()
? Or where it is used?
I mean near InitTaskfile
. I missed that the value was being passed in from cmd/task/main.go
.
How about this:
InitTaskfile()
accepts apath
. Could be a directory OR a file.- In
cmd/task/task.go
, we get thewd
as before and only if an entrypoint is defined, wefilepathext.SmartJoin(wd, name)
.- This means, by default we pass in the
wd
unless--taskfile
is given
- This means, by default we pass in the
- In
InitTaskfile()
, we replace the stat check with this:
if fi, err := os.Stat(f); err == nil && !fi.IsDir() { ... }
- This means that we only error if a file already exists
- Below this, we can add a line that will add the default filename if
fi.IsDir()
is true.
The nice thing about this is that it doesn't break the package API. Anyone calling InitTaskfile
with a directory will get the same result as before, but anyone passing a specific file name will now get that specific file. It also means that we're not using the defaultTaskfile
const outside of init.go
, so it can stay where it was and that all the logic for handling default files is encapsulated in the package API, not the CLI.
Hope this makes sense and I haven't missed anything. I've not actually tried any of what I've suggested 😆
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.
yeah that sounds good, I'll give it a try.
if len(flags.Entrypoint) > 0 { | ||
// Replace `*` with `Taskfile` so `*.yaml` results in `Taskfile.yaml` | ||
name = strings.Replace(flags.Entrypoint, "*", "Taskfile", 1) | ||
} |
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.
The syntax of this isn't intuitive to me. A *
usually denotes a wildcard. i.e. A character that can be any value, not a placeholder for a specific value. A better syntax would follow the standards set by various print formatters (%t
) or go templating ({{.DefaultTaskfileName}}
).
The latter is just worse way of writing Taskfile
and while I could be persuaded on % formatting strings, we don't use this anywhere else in Task, and I'm not convinced this functionality is actually that useful.
Second opinions from @andreynering and @vmaerten 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.
My motive was making it as short (and fast) as possible to init a Taskfile.yaml
file: task -it *.yaml
(so I don't have to type in "Taskfile").
That said, I totally understand your points.
Another solution could be to detect just yaml
and prepend Taskfile.
to that, or maybe drop the *
and detect .yaml
instead.
I didn't realize it already existed elsewhere.
as requested to prevent ambiguity with the stdlib package.
Closes #2008.
Task currently does not provide a way to specify which filename to use when initializing a file (hardcoded as
Taskfile.yml
), forcing users to rename the file/extension after creation.This PR fixes that by making the
--taskfile
flag specify which file name (and path) is used when writing the Taskfile.The first
*
that is found in the filename will be replaced by "Taskfile", so thattask -it *.yaml
can be used to create the fileTaskfile.yaml
.This PR also includes some updates to the docs:
Documentation updates
(CLI Reference page)
Flags
header being inside the Tip admonition:--taskfile
flag entry:Default Filenames
section:Status
Awaiting the merge of #2011 as it will create conflicts with this.