Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Removes previously introduced (now unused) workflow helper functions and their tests as part of the dead-code cleanup series.
Changes:
- Removed several unused validation helpers (and associated tests) from
validation_helpers*. - Removed unused map helper utilities from
map_helpers.go. - Removed unused engine helper utilities (and associated tests) from
engine_helpers*.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/validation_helpers_test.go | Deletes tests for removed validation/map helper utilities. |
| pkg/workflow/validation_helpers.go | Removes unused exported validation helpers and cleans related imports/docs list. |
| pkg/workflow/map_helpers.go | Removes unused map helper utilities and cleans related imports/docs list. |
| pkg/workflow/engine_helpers_test.go | Deletes tests for removed engine helper utilities. |
| pkg/workflow/engine_helpers.go | Removes unused engine helper utilities. |
Comments suppressed due to low confidence (2)
pkg/workflow/engine_helpers.go:273
- This comment references
GetHostedToolcachePathSetup(), but that function has been removed in this PR. Update the documentation here to avoid pointing readers to a non-existent helper (e.g., describe the behavior directly or reference the current PATH-setup mechanism).
// GetNpmBinPathSetup returns a simple shell command that adds hostedtoolcache bin directories
// to PATH. This is specifically for npm-installed CLIs (like Claude and Codex) that need
// to find their binaries installed via `npm install -g`.
//
// Unlike GetHostedToolcachePathSetup(), this does NOT use GH_AW_TOOL_BINS because AWF's
// native chroot mode already handles tool-specific paths (GOROOT, JAVA_HOME, etc.) via
pkg/workflow/validation_helpers_test.go:316
- The PR title/description says this change removes dead functions/tests from
engine_helpers.go, but this PR also removes validation/map helper functions and a large block of tests invalidation_helpers_test.go. Please update the PR description (or split the change) so the scope matches the actual diff.
// TestDirExists tests the fileutil.DirExists helper function
func TestDirExists(t *testing.T) {
t.Run("empty path returns false", func(t *testing.T) {
result := fileutil.DirExists("")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
7
to
11
| // # Available Helper Functions | ||
| // | ||
| // - validateIntRange() - Validates that an integer value is within a specified range | ||
| // - ValidateRequired() - Validates that a required field is not empty | ||
| // - ValidateMaxLength() - Validates that a field does not exceed maximum length | ||
| // - ValidateMinLength() - Validates that a field meets minimum length requirement | ||
| // - ValidateInList() - Validates that a value is in an allowed list | ||
| // - ValidatePositiveInt() - Validates that a value is a positive integer | ||
| // - ValidateNonNegativeInt() - Validates that a value is a non-negative integer | ||
| // - validateMountStringFormat() - Parses and validates a "source:dest:mode" mount string | ||
| // |
There was a problem hiding this comment.
The file-level docs still describe helpers like string validation and list membership, but those helpers were removed in this PR. Please update this header section so it accurately reflects the remaining exported/available helpers in this file.
See below for a potential fix:
// such as integer range validation, mount string format validation, list formatting,
// and target repository slug validation. These utilities are used across multiple
// workflow configuration validation functions.
//
// # Available Helper Functions
//
// - validateIntRange() - Validates that an integer value is within a specified range
// - validateMountStringFormat() - Parses and validates a "source:destination:mode" mount string
// - formatList() - Formats a slice of strings as a human-readable, comma-separated list
// - validateTargetRepoSlug() - Validates that a target-repo slug is not the wildcard "*"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes 5 dead functions and their exclusive tests from engine_helpers.go:
Also removes 8 corresponding test functions from engine_helpers_test.go.
All kept tests pass. Part of the ongoing dead code removal series.