-
-
Notifications
You must be signed in to change notification settings - Fork 70
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
Re-enable golangci-lint.yml, turn on linters, and lint. #343
Conversation
@@ -49,11 +48,6 @@ import ( | |||
|
|||
// https://en.wikipedia.org/wiki/Pileup_format | |||
|
|||
var ( | |||
gzipReaderFn = gzip.NewReader |
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.
I think we started doing this to make it easier to mock tests?
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.
I think the big changes to the parsers remove this anyway - since gzip gets tossed into generics, its easier to test
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.
I think we started doing this to make it easier to mock tests?
IMO, you shouldn't need to reach into a module and change its state to mock it, especially given how Go handles errors. We should favor onion architecture keep I/O as high up in the call chain as possible. This allows inner functions to be pure. The gzipReaderFn
variable was unused anyway, that's why I removed it.
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.
Go's interfaces are the solution for testing, IMO. If a function/object needs to perform I/O operations, it should take io.Reader
and io.Writer
interfaces as an argument/field. These interfaces can then be easily be mocked:
import "bufio"
import "bytes"
const someData = ";alskjdf;lkajsdlf"
func main() {
var b bytes.Buffer
mockedWriter := bufio.NewWriter(&b)
FunctionThatTakesAWriter(mockedWriter)
got := b.String()
mockedReader := bytes.NewReader([]byte(someData))
FunctionThatTakesAReader(mockedReader)
}
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.
I agree here! Much more readable too. This is how the bio
parsers work, and is how I mock test to completion fasta
in ioToBio.
@@ -213,19 +211,12 @@ func WriteMulti(sequences []Genbank, path string) error { | |||
// Build builds a GBK byte slice to be written out to db or file. | |||
func Build(gbk Genbank) ([]byte, error) { | |||
gbkSlice := []Genbank{gbk} | |||
multiGBK, err := buildMultiNth(gbkSlice, -1) | |||
multiGBK, err := BuildMulti(gbkSlice) |
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.
@Koeng101 wasn't buildMultiNth
meant to deal with some annoying corner case?
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.
Hmmm not entirely sure, I don't quite remember
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.
The build functions will mostly be moved into bio
once we get there though, I just haven't started on genbank compatibility because it's gonna be quite sizable and deserves its own PR
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.
The underlying logic is the same, it was just being buried a few function calls deep for some reason.
Before: Build(gbk)
makes a single element slice gbks
and passes it to BuildMulti(gbks)
, which immediately calls buildMultiNth(gbks, -1)
. The count
argument is unused in buildMultiNth()
and all its clients pass -1
as a value anyway.
After: Build(gbk)
makes a single element slice gbks
and passes it to BuildMulti(gbks)
, which holds same the logic buildMultiNth()
used to.
IMHO, this (Build()
calling BuildMulti()
) is a little backward, but it seems reasonable to avoid having to append
results.
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 good! Mostly just style changes (obviously, since this turns on linter), and the non-style changes make sense.
@@ -49,11 +48,6 @@ import ( | |||
|
|||
// https://en.wikipedia.org/wiki/Pileup_format | |||
|
|||
var ( | |||
gzipReaderFn = gzip.NewReader |
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.
I agree here! Much more readable too. This is how the bio
parsers work, and is how I mock test to completion fasta
in ioToBio.
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.
No description provided.