polish some line, change setup of golangci-lint and fix tests #12
Conversation
dasrick
commented
Apr 8, 2020
•
dasrick
commented
- polish some line (hints by golangci-lint)
- change setup of golangci-lint (cyclo)
- fix tests
| // bail if a completely nil section provided | ||
| if s == nil { | ||
| return fmt.Errorf("AddSection: nil MessageCardSection received") | ||
| return fmt.Errorf("func AddSection: nil MessageCardSection received") |
There was a problem hiding this comment.
@dasrick Do you feel that all function references in error messages like this one should have the func prefix? I hadn't really thought about it until now.
| case s.StartGroup != false: | ||
| case s.Markdown != false: | ||
| case s.StartGroup: | ||
| case s.Markdown: |
There was a problem hiding this comment.
@dasrick I completely get why this was changed. I started adding checks at the top of the switch statement and just went down from there only changing up the expected values as I went. Good catch.
| mcs.Images = append(mcs.Images, &img) | ||
|
|
||
| mcs.Images = append(mcs.Images, &img) // nolint:scopelint | ||
| } |
There was a problem hiding this comment.
Did I make a mistake here? I wasn't sure what the scopelint check did initially, but I looked it up (https://github.com/kyoh86/scopelint) and I see that it guards against unintentional variable reassignment/use. I'll take another look at this to make sure I've not accidentally done that here.
There was a problem hiding this comment.
I dug further into this and I think the linting error is probably valid. What do you think of this replacement code?
index 84c8c46..be90bc5 100644
--- a/messagecard.go
+++ b/messagecard.go
@@ -239,7 +239,7 @@ func (mcs *MessageCardSection) AddFactFromKeyValue(key string, values ...string)
// provide a photo gallery inside a MessageCard section.
func (mcs *MessageCardSection) AddImage(sectionImage ...MessageCardSectionImage) error {
- for _, img := range sectionImage {
+ for index, img := range sectionImage {
if img.Image == "" {
return fmt.Errorf("cannot add empty image URL")
}
@@ -248,7 +248,7 @@ func (mcs *MessageCardSection) AddImage(sectionImage ...MessageCardSectionImage)
return fmt.Errorf("cannot add empty image title")
}
- mcs.Images = append(mcs.Images, &img)
+ mcs.Images = append(mcs.Images, §ionImage[index])
}
Instead of referencing the loop variable we're just using the index of the range loop to pull from the values passed into the method, which I presume are more stable based on the linter's complaint.
| @@ -111,5 +107,4 @@ func IsValidMessageCard(webhookMessage MessageCard) (bool, error) { | |||
| } | |||
|
|
|||
| return true, nil | |||
|
|
|||
There was a problem hiding this comment.
Should I try to prune all extra whitespace from PRs that I submit? I tend to use it liberally, but I understand that some tooling and projects wish to eliminate it in order to condense the code.
Error from `scopelint` linter: "Using a reference for the variable on range scope `img`" Fix: Use index from range loop to *reach into* the slice argument to retrieve the address of those values instead of using the range loop variable, which likely would have been a subtle source of bugs later on. refs #6, #11, #12
Error from `scopelint` linter: "Using a reference for the variable on range scope `img`" Fix: Use index from range loop to *reach into* the slice argument to retrieve the address of those values instead of using the range loop variable, which likely would have been a subtle source of bugs later on. refs #6, #11, #12
Error from `scopelint` linter: "Using a reference for the variable on range scope `img`" Fix: Use index from range loop to *reach into* the slice argument to retrieve the address of those values instead of using the range loop variable, which likely would have been a subtle source of bugs later on. refs #6, #11, #12
Error from `scopelint` linter: "Using a reference for the variable on range scope `img`" Fix: Use index from range loop to *reach into* the slice argument to retrieve the address of those values instead of using the range loop variable, which likely would have been a subtle source of bugs later on. refs #6, #11, #12
