-
Notifications
You must be signed in to change notification settings - Fork 44
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
testutil/pkgdata: Add TarEntry shorthand constructors #86
Conversation
c3b2567
to
a76d63c
Compare
@niemeyer I've added lots of tests using specially crafted packages in later commits. E.g.:
These shortcut IMHO keep the test case definitions with custom packages a little bit more readable than if I defined the structs directly. It also avoids too much indentation, e.g. here when defining a case with multiple archives:
(These commits still use |
a76d63c
to
4291729
Compare
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.
qq: isn't there existing testing code which should then be replaced by these new shorthands?
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.
Looks great and useful!
internal/testutil/pkgdata.go
Outdated
@@ -274,3 +274,35 @@ func MustMakeDeb(entries []TarEntry) []byte { | |||
} | |||
return data | |||
} | |||
|
|||
func REG(mode int64, path, content string) TarEntry { |
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.
nit: These all caps shorthand versions does not feel quite right to me.
I do not think these fall under initialisms or recognised abbreviations, which should be all caps. Instead I think these closer match some of the shorthand forms as found in, for example, the https://pkg.go.dev/syscall package.
I would take some inspiration from these examples:
Dup()
https://pkg.go.dev/syscall#Dup
Acct()
https://pkg.go.dev/syscall#Acct
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.
Maybe even File()
, Dir()
and Link()
would read nicely.
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.
Can we reach a compromise and use Reg()
, Dir()
and Lnk()
from tar nomenclature? I understand the desire for readable names, but per my commit message:
The reason for the 3 letter names and the order of arguments is to make
the list of paths aligned on the same column in tarball definitions.
in this case I'd much rather have readable lists of paths. In followup commit there's going to be a lot of such lists in test cases. Once the reader grasps the meaning of the function names, it's much easier to follow the test cases written as such
Dir(0755, "./data/"),
Reg(0600, "./data/document.txt", "words words words"),
Lnk(0777, "./data/document", "document.txt"),
Reg(0777, "./data/ebook1", "tale"),
Reg(0777, "./data/ebook2", "another tale"),
Reg(0777, "./data/ebook3", "manual"),
Dir(0755, "./data/pdfs/"),
Reg(0777, "./data/pdfs/lectures1.pdf", "physics"),
Reg(0777, "./data/pdfs/lectures2.pdf", "chemistry"),
Reg(0777, "./data/pdfs/lectures3.pdf", "history"),
instead of misaligned
Dir(0755, "./data/"),
File(0600, "./data/document.txt", "words words words"),
Lnk(0777, "./data/document", "document.txt"),
File(0777, "./data/ebook1", "tale"),
File(0777, "./data/ebook2", "another tale"),
File(0777, "./data/ebook3", "manual"),
Dir(0755, "./data/pdfs/"),
File(0777, "./data/pdfs/lectures1.pdf", "physics"),
File(0777, "./data/pdfs/lectures2.pdf", "chemistry"),
File(0777, "./data/pdfs/lectures3.pdf", "history"),
I've pushed the change to camel cased names. If you are fine with this compromise, please resolve the thread.
4291729
to
e0e385b
Compare
2bc9b0d
to
a93e638
Compare
Per discussion in one of the previous PRs, this is not wanted. See the discussion in the PR #42 and in our Mattermost channel. In that PR I originally changed existing code to use the added function, but Gustavo rejected changes to the existing code. |
Introduce shorthand constructors for testutil.TarEntry structures. These are Reg(), Dir() and Lnk() functions. The rationale for this addition is to make the test case definition less verbose. There are other changes in the queue that construct custom packages to test various test cases. In all these new tests (and old ones as well) we only care about file's type, path, mode and content. With these shorthand constructors and function aliases we can define tar entries like: Dir(0755, "./data/"), Reg(0600, "./data/document.txt", "words words words"), Lnk(0777, "./data/document", "document.txt"), Instead of: testutil.TarEntry{ Header: tar.Header{ Name: "./data/", Mode: 0755, }, }, testutil.TarEntry{ Header: tar.Header{ Name: "./document.txt", Mode: 0600, }, Content: []byte("words words words"), }, testutil.TarEntry{ Header: tar.Header{ Name: "./document.txt", Mode: 0777, Linkname: "document.txt", }, }, The reason for the 3 letter names and the order of arguments is to make the list of paths aligned on the same column in tarball definitions. These function only create barebone TarEntry. It'll still get adjusted when passed through fixupTarEntry().
a93e638
to
f836ca8
Compare
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.
Thanks, and although this is a trivial change, just wanted to say thanks to the earlier reviewers as well. Good comments, and made this an easy one in.
Introduce shorthand constructors for testutil.TarEntry structures. These
are REG(), DIR() and LNK() functions.
The rationale for this addition is to make the test case definition less
verbose. There are other changes in the queue that construct custom
packages to test various test cases. In all these new tests (and old
ones as well) we only care about file's type, path, mode and content.
With these shorthand constructors and function aliases we can define tar
entries like:
Instead of:
The reason for the 3 letter names and the order of arguments is to make
the list of paths aligned on the same column in tarball definitions.
These function only create barebone TarEntry. It'll still get adjusted
when passed through fixupTarEntry().