-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix(taskfiles): Use empty list as default for optional task arguments passed via a ref
(fixes 14).
#15
Conversation
…passing it to a task via a ref (fixes 14).
WalkthroughThe pull request introduces changes to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ValidateChecksum
participant ComputeChecksum
User->>ValidateChecksum: Trigger validate-checksum task
ValidateChecksum->>ValidateChecksum: Reference EXCLUDE_PATHS
ValidateChecksum->>User: Validation result
User->>ComputeChecksum: Trigger compute-checksum task
ComputeChecksum->>ComputeChecksum: Compute checksum
ComputeChecksum->>User: Checksum result
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
taskfiles/utils.yml (1)
53-53
: Consider enhancing the parameter documentation.While the fix is solid, it would be beneficial to update the parameter documentation to explicitly mention the default behaviour:
-# @param {[]string} [EXCLUDE_PATHS] A list of paths, relative to `DATA_DIR`, to exclude from the -# checksum. +# @param {[]string} [EXCLUDE_PATHS] A list of paths, relative to `DATA_DIR`, to exclude from the +# checksum. Defaults to an empty list if not specified.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
taskfiles/utils.yml
(1 hunks)
🔇 Additional comments (1)
taskfiles/utils.yml (1)
53-53
: LGTM! Excellent fix for the Go iteration issue.
The change to use default (list)
effectively addresses the issue where undefined variables were being treated as empty strings. This ensures proper handling in Go by defaulting to an empty list 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.
Makes sense to me. I will merge this unto my open clp-ffi-js PR, which will provide more confirmation
@@ -50,7 +50,7 @@ tasks: | |||
vars: | |||
DATA_DIR: "{{.DATA_DIR}}" | |||
EXCLUDE_PATHS: | |||
ref: ".EXCLUDE_PATHS" | |||
ref: "default (list) .EXCLUDE_PATHS" |
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.
I looked at https://taskfile.dev/reference/templating/#default-functions and believe our usage matches the explanations there.
Description
As #14 describes, the behaviour of
task
has changed w.r.t. passing optional (undefined) variables viaref
. It seems thattask
will now treat it as an empty string rather than leaving it as an undefined variable. Go is unhappy when trying to iterate over a variable which contains an empty string, but Go seems to skip iterating entirely if the variable is undefined.I'll report this issue to the
task
devs, but in the meantime, this PR defines a default empty list for task parameters that we pass via theref
attribute.Validation performed
task lint:check
currently fails inclp-ffi-js
task lint:check
now succeeds.Summary by CodeRabbit
EXCLUDE_PATHS
variable in thevalidate-checksum
task for improved clarity.compute-checksum
task.