pkg/archive: make CanonicalTarNameForPath an alias for filepath.ToSlash#44060
Merged
thaJeztah merged 2 commits intomoby:masterfrom Sep 21, 2022
Merged
pkg/archive: make CanonicalTarNameForPath an alias for filepath.ToSlash#44060thaJeztah merged 2 commits intomoby:masterfrom
thaJeztah merged 2 commits intomoby:masterfrom
Conversation
filepath.ToSlash is already a no-op on non-Windows platforms, so there's no need to provide multiple implementations. We could consider deprecating this function, but it's used in the CLI, and perhaps it's still useful to have a canonical location to perform this normalization. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Now that CanonicalTarNameForPath is an alias for filepath.ToSlash, they were mostly redundant, and only testing Go's stdlib. Coverage for filepath.ToSlash is provided through TestCanonicalTarName, which does a superset of CanonicalTarNameForPath, Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
added
status/2-code-review
kind/refactor
thaJeztah
commented
Aug 30, 2022
Comment on lines
-26
to
-28
| func TestCanonicalTarNameForPath(t *testing.T) { | ||
| cases := []struct{ in, expected string }{ | ||
| {"foo", "foo"}, |
Member
Author
There was a problem hiding this comment.
I kept the removal of the tests as a separate commit; if there's concerns about not having these tests (they should be covered by TestCanonicalTarName below), I can drop the second commit.
thaJeztah
commented
Aug 30, 2022
Comment on lines
+561
to
+563
| func CanonicalTarNameForPath(relativePath string) string { | ||
| return filepath.ToSlash(relativePath) | ||
| } |
Member
Author
There was a problem hiding this comment.
FWIW; #37356 (comment) already suggested to (deprecate and) remove this function altogether, which is something that still could be considered.
vvoland
approved these changes
Sep 1, 2022
Member
Author
|
@tonistiigi @corhere PTAL |
corhere
approved these changes
Sep 7, 2022
Member
Author
|
let me bring this one in |
thaJeztah
changed the title
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
3 participants
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.
pkg/archive: make CanonicalTarNameForPath and alias for filepath.ToSlash
filepath.ToSlash is already a no-op on non-Windows platforms, so there's no
need to provide multiple implementations.
We could consider deprecating this function, but it's used in the CLI, and
perhaps it's still useful to have a canonical location to perform this normalization.
pkg/archive: remove tests for CanonicalTarNameForPath
Now that CanonicalTarNameForPath is an alias for filepath.ToSlash, they were
mostly redundant, and only testing Go's stdlib. Coverage for filepath.ToSlash is
provided through TestCanonicalTarName, which does a superset of CanonicalTarNameForPath,