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

[WIP] Allow shorter syntax for tasks with default configuration #240

Closed
wants to merge 6 commits into from
Closed

[WIP] Allow shorter syntax for tasks with default configuration #240

wants to merge 6 commits into from

Conversation

jaedle
Copy link
Contributor

@jaedle jaedle commented Aug 23, 2019

See #194

After doing some coding/research how to implement this, I have a few questions:

  1. If I got it right, this is a new feature only for version 3 taskfiles?
  2. What should happen if an older version (i.e. version 2) is used with that notation? Should that result in an parsing error? - This would mean like having a switch inside of internal/taskfile/taskfile.go for disallow a shorter syntax for older taskfile versions?
  3. Because the unmarshalling has now different source structures (unmarshal from task structure, umarshal an array of strings, unmarshal a string) how should be dealt with the possible errors? I think giving a good error message may be complicated, because we can't guess which synrax should be used. For an approach to unmarshal different types within go-yaml see Parsing array of string or single string go-yaml/yaml#100 (comment)

(I will squash this branch before review)

@andreynering andreynering added this to the v3 milestone Aug 25, 2019
@andreynering
Copy link
Member

andreynering commented Sep 2, 2019

Hi @jaedle, thanks for opening this PR!

Regarding 1) and 2), I think would not be a that big deal to allow the shorter syntax on v2, too. (Even then, this should still target the v3 branch). That's what we've been done with other syntax alternatives, too.

That said, if we find a easy/right way to only allow it on v3, it'd be the ideal scenario. If you're unsure about how to do that, it could be pulled to a future fix/PR.

Regarding 3), we already do that for other data structures. In short, what you should do is:

  1. If the value is a string slice, assign it to task.Cmds and keep other attributes zeroed/default;
  2. The same if the value is a string, just wrap it into a string slice and assign it to task.Cmds.

Does that makes sense?

Copy link
Member

@andreynering andreynering left a comment

Choose a reason for hiding this comment

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

Good job so far 👏 👏 👏

I did a suggestion on how to simplify func (t *Task) UnmarshalYAML a bit, though. Hope that makes sense.

@@ -40,3 +40,61 @@ func (tf *Taskfile) UnmarshalYAML(unmarshal func(interface{}) error) error {
}
return nil
}

func (t *Task) UnmarshalYAML(unmarshal func(interface{}) error) error {
var task struct {
Copy link
Member

Choose a reason for hiding this comment

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

Hey @jaedle,

IMHO you don't need to copy the entire task struct into this function. Just try to marshal the string and []string types first, and none work then unmarshal to the t variable directly. 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Oops, I just remembered that we also have alternative command syntaxes, like { cmd: "echo foo", silent: true } for example.

That's easy to do, though. Just check for taskfile.Cmd and []taskfile.Cmd instead of for string and []string. All variations should just work.

version: '3'

tasks:

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@@ -20,5 +20,5 @@ type Task struct {
Silent bool
Method string
Prefix string
IgnoreError bool `yaml:"ignore_error"`
Copy link
Member

Choose a reason for hiding this comment

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

Given the suggestion made on func (t *Task) UnmarshalYAML, we should re-add this reflect tag.

andreynering added a commit that referenced this pull request Dec 8, 2019
Closes #194
Closes #240

Co-authored-by: Jaedle <dennis.jekubczyk@gmail.com>
@andreynering
Copy link
Member

Hi @jaedle,

I just manually merged your work on v3 with some changes on b7b752b.

Thanks a lot for contributing!

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.

2 participants