Feature: loon nix build
#54
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network.
Check the status or cancel PullRequest code review here - or - cancel by adding [!pr] to the title of the pull request.
There was a problem hiding this comment.
| 1 Message | |
|---|---|
| 📚 | It looks like the description for this pull request is either blank or very short. Adding a high-level summary will help our reviewers provide better feedback. Feel free to include questions for PullRequest reviewers and make specific feedback requests. |
There was a problem hiding this comment.
One of the more challenging things when starting a Go project is structuring it for change. With that in mind, and given that you have some tooling already set up, I highly recommend and editor experience and GitHub workflow which incorporates golangci-lint.
Coincidentally, I spent a few hours last night working on this for my own work and I will share what I did. There are many, examples, this one is mine:
run:
deadline: 5m
skip-dirs:
# Autogenerated files take too much time and memory to load,
# even if we skip them with skip-dirs.
# So we define this tag and use it in the autogenerated files.
build-tags:
- codeanalysis
skip-files:
- path/to/ignore
issues:
exclude-rules:
- path: path/to/ignore
linters:
- govet
# output configuration options
output:
# colored-line-number|line-number|json|tab|checkstyle|code-climate, default is "colored-line-number"
format: colored-line-number
# print lines of code with issue, default is true
print-issued-lines: true
# print linter name in the end of issue text, default is true
print-linter-name: true
linters:
# Run 'golangci-lint linters' to see them. This option implies option --disable-all
enable:
- gocritic
- unconvert
- goconst
- gosimple
- misspell
disable:
- gochecknoglobals
- lll
- golint
- scopelint
presets:
- bugs
- unused
- format
- complexity
fast: false
linters-settings:
govet:
check-shadowing: true
golint:
min-confidence: 1
maligned:
suggest-new: true
misspell:
locale: US
gocyclo:
min-complexity: 24
dupl:
threshold: 60
goconst:
min-len: 7
min-occurrences: 4
gocritic:
enabled-tags:
- performance
- style
- experimental
disabled-checks:
- docStub
- ifElseChain
- wrapperFuncYou would create your own and execute it as golangci-lint run --build-tags=codeanalysis ./... . I use a build tag because I generate code and prefer to not even have it discovered (there is a difference between skipped and ignored) using build tags in the file. I hope it is useful for your efforts.
All other feedback inline. Feel free to reply directly if you have any questions or if there's anything specific you'd like feedback on - ty!
Reviewed with ❤️ by PullRequest
| @@ -32,6 +35,40 @@ func (p Package) DerivationPackages() string { | |||
| return buf.String() | |||
| } | |||
|
|
|||
| func (p Package) Build() error { | |||
There was a problem hiding this comment.
The derivation struct uses a pointer receiver. Using the pointer receiver here would be my expectation so you are working with the current value at the address p instead of a copy, particularly since it is used in the file creation.
However, there are caveats to the value of using a pointer which may outweigh its benefit. Here's a good piece by Dave Cheney for deeper insight: https://dave.cheney.net/tag/receivers
| ; | ||
| }` | ||
| buf := bytes.NewBuffer([]byte{}) | ||
| t, err := template.New("nix").Parse(tmpl) |
| // TODO(andremedeiros): figure out a better way | ||
| panic(err) | ||
| } | ||
| t.Execute(buf, p) |
| panic(err) | ||
| } | ||
| t.Execute(buf, p) | ||
| f, _ := ioutil.TempFile("", "default.nix") |
There was a problem hiding this comment.
You definitely want to check for an error here, per the documentation: https://golang.org/src/io/ioutil/tempfile.go?h=TempFile#L50
| t.Execute(buf, p) | ||
| f, _ := ioutil.TempFile("", "default.nix") | ||
| f.Write(buf.Bytes()) | ||
| f.Close() |
There was a problem hiding this comment.
ISSUE: G104 (Severity: Medium)
Errors unhandled.
Remediation:
It is expected to check for errors and in this case there is still the matter of execution which happens in the stanza below. If there is an error, should this code attempt to execute?
🤖 powered by PullRequest Automation 👋 verified by Denis R
There was a problem hiding this comment.
Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard.
No description provided.