fix(ls-metrics): report instead of panicking when nothing lints - #1144
Open
VXNCXNX wants to merge 1 commit into
Open
fix(ls-metrics): report instead of panicking when nothing lints#1144VXNCXNX wants to merge 1 commit into
VXNCXNX wants to merge 1 commit into
Conversation
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.
What's broken
vale ls-metricspanics on any path that lints to zero files.The guard above it is
system.FileExists(args[0]), which isos.Stat(f) == niland therefore true for a directory.linter.Lintreturns an empty slice when nothing matches, andlinted[0]is unchecked.A single non-lintable file is fine,
vale ls-metrics pic.pngprints{}, which is exactly what makes this look covered. The reachable cases are an empty directory, a directory of only empty subdirectories, and a directory whose files all sit under a default-ignored path such asnode_modulesor.git.The fix
Guard the index and return an error naming the situation:
An error rather than
{}, becausetestdata/e2e/metrics.yamlalready pinsls-metrics test.txtto exit 0 with{}for a real file Vale read and found no metrics in. Reusing{}for "Vale read nothing at all" would conflate the two and hide the mistake.core.NewE100is what the sibling commands incommand.gouse, andhandleErrorrenders it and exits 2.Verification
An
empty-directorycase intestdata/e2e/metrics.yaml, run byinternal/e2e/e2e_test.go. It uses the suite's inlinefiles:mechanism, since git cannot check in an empty directory, and puts the only file undernode_modulesso the walker skips it and the lint comes back empty.With the guard reverted the case fails with the panic above instead of the expected output.
go test ./...passes, andgolangci-lint run ./cmd/...reports the same single pre-existing gosec issue with and without the change.