Repository navigation
ST1017: possible false positive in non-if conditions #410
Description
I have some code that looks like this:
// Render the tabnav.
err := htmlg.RenderComponents(w, tabnav{
Tabs: []tab{
{
Content: iconText{Icon: octicon.Person, Text: "Overview"},
URL: "/about",
Selected: "/about" == req.URL.Path,
},
{
Content: iconText{Icon: octicon.DeviceDesktop, Text: "Setup"},
URL: "/about/setup",
Selected: "/about/setup" == req.URL.Path,
},
},
})
if err != nil {
return err
}I get don't use Yoda conditions (ST1017) reports on "/about" == req.URL.Path and "/about/setup" == req.URL.Path lines.
I'm not sure if it's a true positive in this case. I can see how yoda conditions in if statements are not worth having, and thus worth reporting. But in this case, having the "/about" and "/about/setup" alignment improves readability IMO.
To me, it's not clear that swapping the comparison order is an improvement:
{
Content: iconText{Icon: octicon.Person, Text: "Overview"},
URL: "/about",
Selected: req.URL.Path == "/about",
},
{
Content: iconText{Icon: octicon.DeviceDesktop, Text: "Setup"},
URL: "/about/setup",
Selected: req.URL.Path == "/about/setup",
},What do you think?
If we agree on this being a false positive, maybe a broad fix is to limit scope to if statements that cannot rely on indentation the way struct literals can, and a more narrow one is to ignore struct literals with aligned fields.
Activity
ainar-g commentedon Feb 4, 2019
To be honest, both seem like a bad style to me, as both have magical strings. Better:
dmitshur commentedon Feb 5, 2019
I didn't expect it to have an effect on ST1017, but using such constants makes
staticcheckno longer emit ST1017 on those lines.I agree using consts for paths is better, and that was the long term plan. Using string literals was a part of the first prototype. Perhaps I'll expedite the process of starting to use consts.
dominikh commentedon Feb 7, 2019
Well, the definition of a yoda condition is
<literal> <binop> <non-literal>.dmitshur commentedon Feb 7, 2019
I see. I thought it would be
<const> <binop> <non-const>. Why isn’t it?dominikh commentedon Feb 7, 2019
Well, because that's not the pattern we're trying to flag :-) – lit op non-lit is common in languages where assignment is an expression and pretty much always an anti-pattern in Go (if we ignore your case). Most often you'll see it with very basic literals, such as
0(if 0 == ret).What side of an expression a const should be on seems a lot more opinionated, and will depend on things other than trying to avoid an accidental assignment.
dominikh commentedon Feb 15, 2019
I'm going to close this for now. It's a rare and weird enough edge case that I am comfortable recommending a
//lint:ignore, writing your code differently, or disabling the check altogether.I will reconsider this if more people run into the same issue.