stephen-soltesz
commented
7 years ago Reviewed 1 of 4 files at r1. Review status: 1 of 4 files reviewed at latest revision, 3 unresolved discussions.
unembargo.go, line 61 at r2 (raw file):
} func (nc *config) NewConfig(privateBucketName, publicBucketName string) {
I suggest using a method signature like:
func NewConfig(privateBucketName, publicBucketName string) *config {
...
}The NewConfig function can allocate a new config instance and return it populated with the bucket names.
unembargo.go, line 70 at r2 (raw file): Copying thread from pr #7.
From soltesz: Please only use type casts when necessary. Like this: https://play.golang.org/p/W1dPo1IBvX
Also, I'm sorry to have missed it before; please use use mixed case variable names rather than underscores. This is the convention in Go https://golang.org/doc/effective_go.html#mixed-caps
From yachang: If I do not use type casts, there will be errors like:
mismatched types int and time.Month
Right. I said "only when necessary". Did you look at the example? It is necessary for time.Month but not the other values.
unembargo_test.go, line 14 at r2 (raw file):
*/ package embargo
I believe we can define the test package differently from the package to guarantee that the tests use the public interface of the package under test.
So, here please use package embargo_test and update the tests to use the public embaro interface.
Comments from Reviewable
yachang
Fix the rebase issues.
This change is