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

Support any variables #1421

Merged
merged 13 commits into from Dec 21, 2023
Merged

Support any variables #1421

merged 13 commits into from Dec 21, 2023

Conversation

pd93
Copy link
Member

@pd93 pd93 commented Dec 3, 2023

This is the first iteration of the "Any Variables" experiment that allows users to use any type in their Taskfile variables. In this PR:

  • Adds the TASK_X_ANY_VARIABLES experiment flag
  • Renames Var.Static -> Var.Value
  • Changes Var.Value to an any type
  • Unmarshal into any type instead of just strings when the experiment flag is enabled.
  • Updated schema to support objects and arrays in vars
  • Added some experiment docs
  • for supports arrays when looping over a variable
  • Added an error message when trying to use sh with the experiment enabled
  • Added new syntax for dynamic variables

There are no tests in this PR as its an experiment. However, I have added a Taskfile in testdata/vars/any which can be used to try out the functionality:

go run ./cmd/task --dir testdata/vars/any


The main things I'm looking for feedback on are:

  • Bugs/errors (obviously)
  • Missing functionality (Is there anything else we could do now that we support any variables)
  • Dynamic variables
    • Dynamic variables previously used a subkey (sh) to define the command. This is incompatible with map variables as they would be defined the same way. This means that dynamic variables are now defined by starting a string variable with the $ character instead.
    • Do we like/dislike this?
    • An alternative would be to not allow map variables in this experiment and keep the existing sh syntax. This would make this change non-breaking and it could be merged into v3 rather than waiting for a major version bump. However, you won't be able to define variables as maps at all.
    • Feedback on how useful map variables are would be help weigh up the pros/cons of each option.

@pd93 pd93 linked an issue Dec 3, 2023 that may be closed by this pull request
3 tasks
@pd93 pd93 removed a link to an issue Dec 3, 2023
3 tasks
@pd93 pd93 marked this pull request as draft December 3, 2023 16:01
@pd93 pd93 marked this pull request as ready for review December 17, 2023 03:42
@pd93
Copy link
Member Author

pd93 commented Dec 17, 2023

Done some more bug bashing today 🐞 This is starting to feel somewhat stable from my end.

Had a bizarre GH actions issue with the linter, but updating to the latest golangci-lint release seems to have fixed it 🤷

@andreynering Would love to get this experiment merged as a draft for the next release if possible. Any immediate thoughts? I think the main decision for this experiment is going to be whether we should support maps (and remove sh) or disallow them for now (which might allow us to release this into v3).

@andreynering andreynering linked an issue Dec 21, 2023 that may be closed by this pull request
3 tasks
@andreynering
Copy link
Member

@pd93 Great work so far!

I think the easier decision for now is to remove support for maps so we can release the experiment for now. We can then decide later if we want to add that support back. I'd say that arrays will probably be a lot more used and useful than maps, so supporting that is the priority, anyway.

And yes, I can postpone the next release to tomorrow or Friday if needed, so you have time to address that and it'll be included the release. 😉

@andreynering
Copy link
Member

andreynering commented Dec 21, 2023

Hmm... once actually trying the Taskfile on testdata/vars/any, I get marshal errors on arrays specifically. Other types (ints, bools and maps) work if I comment these lines:

task: Failed to parse Taskfile.yml:
yaml: line 51: cannot unmarshal !!seq into variable

Looks like something easy to fix.

EDIT: Nevermind, I forgot to set TASK_X_ANY_VARIABLES=1. 🤦‍♂️

@andreynering andreynering removed a link to an issue Dec 21, 2023
3 tasks
@pd93 pd93 merged commit 43a2979 into main Dec 21, 2023
11 checks passed
@pd93 pd93 deleted the 1415-support-any-variables branch December 21, 2023 01:55
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.

None yet

2 participants