tools: Add linter rule for goconst.
#398
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #398 +/- ##
===========================================
+ Coverage 64.94% 64.99% +0.04%
===========================================
Files 86 86
Lines 10001 10015 +14
===========================================
+ Hits 6495 6509 +14
Misses 2867 2867
Partials 639 639
|
shahzadlone
force-pushed
the
lone/tools/add-goconst-linter
branch
from
3f87834
to
394c0f9
Compare
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
shahzadlone
requested review from
AndrewSisley,
jsimnz,
fredcarle and
orpheuslummis
and removed request for
AndrewSisley
core/data_test.go
Outdated
| @@ -16,6 +16,28 @@ import ( | |||
| "github.com/stretchr/testify/assert" | |||
| ) | |||
|
|
|||
| const ( | |||
There was a problem hiding this comment.
todo: I really dont think this makes reading the tests easier. Suggest a bit of a rethink and consider skipping the test files for this rule if that is possible
There was a problem hiding this comment.
I am of the same opinion
There was a problem hiding this comment.
dam... I looked at this without thinking about the fact that this is a test file 🤦♂️
I agree. This list of constants doesn't really improve anything here. Maybe a map of the keys instead?
There was a problem hiding this comment.
It might not make tests easier but definitely doesn't make them harder to read either. In my opinion this still gives most of the benefits of having constant defined strings that you would have in non-test code as well. Some of these nice attributes to a new person coming to this test file might be:
-
Better insight to common string literals. For example the key being used on line 429 and on line 65 are the same, it's obvious to spot that this way. However if it was instead this on line 65
"/p/0/0/k2"and this on line 429"/p/0/0/K2"it is very easy to miss the difference (capitalized 'K' on the second one). -
When I first looked at this file I wondered how many unique keys are being used in the test (no good reason why lol), but having them defined in one place it's easy to see.
I would say in this case the linter rule is very easy to add and maintain even for the test files.
However open to hear what others think as well, as it might just be my love for constants making me biased here haha.
@fredcarle @jsimnz opinions?
There was a problem hiding this comment.
To Andy's point, it doesn't make reading the tests easier. Also tests are in isolation of one another so you don't gain much by having global const/var if there are no duplication of the strings within the test functions themselves.
In test files, I'd rather see an array or a map that you can loop through (if it makes sense to do so). Unless there are many instances of the same string in the one function, then it makes sense to assign to a scoped variable.
The more I look at the file to question my own opinion, the less I like to see the constants being use as values to variables that are then used as value parameters to functions. I'll sleep on it and see if I can come up with something helpful in the morning.
There was a problem hiding this comment.
It might not make tests easier but definitely doesn't make them harder to read either.
Yes it does. You can no longer see the values under test and instead have to scroll up (potentially a few hundred lines) to view them. It can also couple tests together if they are mindlessly shared between them. And it creates a meaningless block of nonsense at the top of the file.
There was a problem hiding this comment.
Yes it does. You can no longer see the values under test and instead have to scroll up (potentially a few hundred lines) to view them. It can also couple tests together if they are mindlessly shared between them. And it creates a meaningless block of nonsense at the top of the file.
As that seems like the general consensus, will turn this rule off for tests.
- Removed the linter rule from all test files.
Yes I noticed that when I put the |
For |
Awesome thanks for sharing the tip, will add it to my go development jar of tips. |
shahzadlone
force-pushed
the
lone/tools/add-goconst-linter
branch
from
394c0f9
to
0e68988
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
query/graphql/parser/property.go
Outdated
| package parser | ||
|
|
||
| const ( | ||
| cidProperty = "cid" |
There was a problem hiding this comment.
suggestion: Sorry I'm a bit late on this one, but in my opinion 'Property' is quite ambiguous and we could name these a bit better. Or type alias them perhap?
type InputArgName string
const (
cidInputArgName = InputArgName("cid")
dataInputArgName = ...
)
Or perhaps even move them (and similar items) to a new sub package of client where we hold the public query components.
Sorry about the hassle, but I do not want us to move these some place poorly for the sake of a linter rule. The rules should only ever force us to make things better and we should avoid finding quick satisfactions to them, even if that makes introducing new rules a bit more painful/tedious.
There was a problem hiding this comment.
thought: If we are going to type alias them, we can remove the type from the const name.
The name of the variable should describe its contents, not the type of the contents.
See here for more details.
There was a problem hiding this comment.
Yeah, I dont like the type in name stuff either, but here it could be seen as more of a linguistic coincidence. cidInputArgName is the CID Input Argument Name. Having a global cid is very ambiguous, especially as it easily clashes with local variables (e.g. you have a local/temp var cid that is actually a cid). In C#/JS/etc. I would have scoped/namespaced these in a struct or similar, but unsure how to best handle it in Go:
pub class InputArgNames {
const Cid string = "cid"
}
fn foo() {
...
bar[InputArgNames.Cid] = cid
}
There was a problem hiding this comment.
suggestion: Sorry I'm a bit late on this one, but in my opinion 'Property' is quite ambiguous and we could name these a bit better. Or type alias them perhap?
type InputArgName string const ( cidInputArgName = InputArgName("cid") dataInputArgName = ... )Or perhaps even move them (and similar items) to a new sub package of client where we hold the public query components.
Sorry about the hassle, but I do not want us to move these some place poorly for the sake of a linter rule. The rules should only ever force us to make things better and we should avoid finding quick satisfactions to them, even if that makes introducing new rules a bit more painful/tedious.
You are not late at all and dw it's no hassle, like you mentioned the goal is to make things better even if it takes a longer more tedious route and many changes. I actually didn't know what a more idiomatic approach is so I just tucked them in that file for now.
There was a problem hiding this comment.
A few things I don't like from the current approach is just as you boys also have mentioned that extra 'Type' added to the name, this I usually get around in C++ by using enum classes or a C++ namespace (like Andy mentioned aswell).
Unfortunately, Go doesn't have const enum classes or namespaces but recommends using sub-packages to achieve the namespace effect like we currently have here: https://github.com/sourcenetwork/defradb/blob/develop/query/graphql/schema/types/types.go
However in a discussion with John, he mentioned it can be just flattened outside into the schema package, as we have a few other files with similar constant that would then all require nano subpackages.
Here I can highlight another solution I came across, and we can discuss which one we should adopt until Go comes up with something cleaner...
Using anonymous struct:
var InputPropertyName = struct {
Cid string
Data string
}{
Cid : "cid",
Data : "data",
}
// usage example
var cid := InputPropertyName.Cid
Using anonymous struct with type alias:
type InputPropertyNameType string
var InputPropertyName = struct {
Cid InputPropertyNameType
Data InputPropertyNameType
}{
Cid : InputPropertyNameType("cid"),
Data : InputPropertyNameType("data"),
}
// usage example
var cid := InputPropertyName.Cid
One thing I don't like with above is how you have to type each identifier multiple types. However willing to bite the bullet for that namespacing effect.
LMK, which ones you boys prefer?
- Using anonymous
structwith type alias. - Using anonymous
struct. - Nano-sub packages.
- Separate file in package, like we already have (no namespace).
There was a problem hiding this comment.
I'm pretty against most uses of anonymous structs, especially for globals, since you lose a lot of type information.
I could get behind a more thought out type nano sub package.
I'm also quite fine with keeping these const/var globals in the parser package, but it would be work to properly name them since they are somewhat generic (ie: cid)
So I would prob suggest nano package approach as you described it
There was a problem hiding this comment.
Creating a sub-package seems to be a clear way to create namespace, at the negligible cost of having a bit more clutter and bit more 'distance'.
Otherwise, the status quo doesnt seems bad to me (considering the alternatives): keep them in-package using a naming convention. The suffixProperty, while ambiguous taken alone, seems clear enough in context.
There was a problem hiding this comment.
Alright boys. I've had enough time to debate this with myself that I think I can add my comment.
First, just to get this out of the way, the only times I will use an anonymous struct is for a test function or in rare occasions where that struct is only needed inside a single function, and having a type would be irrelevant.
As for my recommendation for the list of constants, my preferred solution would be to keep the constants here but in the following form:
type queryArg string
const (
typeCID = queryArg("cid")
typeData = queryArg("data")
typeDocKey = queryArg("dockey")
typeDocKeys = queryArg("dockeys")
typeField = queryArg("field")
typeID = queryArg("id")
typeIDs = queryArg("ids")
clauseFilter = queryArg("filter")
clauseGroupBy = queryArg("groupBy")
clauseLimit = queryArg("limit")
clauseOffset = queryArg("offset")
clauseOrder = queryArg("order")
)Otherwise I don't mind a sub package but I think it should be an internal one if we don't plan on making those public.
There was a problem hiding this comment.
As for my recommendation for the list of constants, my preferred solution would be to keep the constants 'here' but in the following form:
Where is here haha? like in the same file that I made? What names would you recommend with the sub-package approach? are you recommending two namespaces? clauses and types packages?
There was a problem hiding this comment.
here is where you have it. Same file.
If you want to use sub-packages, i would use internal/clauses and internal/types
shahzadlone
force-pushed
the
lone/tools/add-goconst-linter
branch
from
0e68988
to
a71aa71
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
shahzadlone
added
the
action/no-benchmark
query/graphql/parser/types/types.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| Cid = string("cid") |
There was a problem hiding this comment.
question: what's the difference between "cid" and string("cid")?
There was a problem hiding this comment.
Helping the compiler infer.
If we don't do either Cid = string("cid") or Cid string = "cid", i.e. I have Cid = "cid" lets say.
Then, the type is shown as const Cid untyped string rather than const Cid string
There was a problem hiding this comment.
Could you change this to Cid string = "cid"? Otherwise it looks like you are converting a string to a string.
There was a problem hiding this comment.
Helping the compiler infer.
The Go compiler doesnt need help here, and dev readability should always trump anything compiler-help-related (maybe c++ templates have a bad rep. here?). The compiler is a tool to help the developer, not the other way around and our build times are really low.
There was a problem hiding this comment.
TBH I personally don't care and do prefer the one that isn't Cid = string("cid") or Cid string = "cid". However I started doing this because @fredcarle made this suggestion when I only had key_1 = "/p/0/0/k1" instead of key_1 string = "/p/0/0/k1" earlier in this PR.
Not sure what the difference here is @fredcarle
There was a problem hiding this comment.
I think I always prefer implicit typing (cant think of any exceptions atm). If the compiler struggles with this then there is a bug in in compiler IMO lol.
In light of what I posted above, there is no bug in the compiler for const cid = "cid" the actual type is not string type, it is actually untyped string, which is the defined behavior. So, would prefer const cid string = "cid" explicitly, to make it a string type.
There was a problem hiding this comment.
Moreover, for our iota constants we currently have them as untyped int which might be fine because otherwise we wouldn't be able to use them as the enum effect we want (for example cannot use CommitSelection as type SelectionType like we are using already)
We have this:
ScanQuery = iota
VersionedScanQuery
NoneSelection = iota
ObjectSelection
CommitSelection
if we had something like:
ScanQuery int = iota
VersionedScanQuery
NoneSelection int = iota
ObjectSelection
CommitSelection
Then we will not be able to do this:
query/graphql/parser/commit.go:57:2: cannot use types.CommitSelection (type int) as type types.SelectionType in return argu
query/graphql/parser/commit.go:84:3: cannot use types.CommitSelection (type int) as type types.SelectionType in field value
query/graphql/parser/mutation.go:107:2: cannot use types.ObjectSelection (type int) as type types.SelectionType in return a
query/graphql/parser/query.go:248:30: cannot use types.ObjectSelection (type int) as type types.SelectionType in argument t
query/graphql/parser/query.go:347:19: cannot use types.VersionedScanQuery (type int) as type types.SelectQueryType in assig
query/graphql/parser/query.go:349:19: cannot use types.ScanQuery (type int) as type types.SelectQueryType in assignment
query/graphql/parser/query.go:433:14: cannot use types.CommitSelection (type int) as type types.SelectionType in assignment
There was a problem hiding this comment.
Actually I have given it some more thought and would actually be in favor of having untyped string while using const. This would give us more freedom to compare of use constants with other types that aren't string (but their underlying type is string).
Consider this :
type a_string = string
type not_a_string string
const (
aString a_string = "_str" // type: `string`
typedString string = "_str" // type: `string`
notString not_a_string = "_str" // type: `not_a_string`
untypedString = "_str" // type: `untyped string`
)
func stringTest() {
// cannot compare typedString == notString (mismatched types string and not_a_string)
string_vs_notString := typedString == notString // Error
untypedString_vs_notString := untypedString == notString
untypedString_vs_string := untypedString == typedString
}
There was a problem hiding this comment.
:) Cheers for posting all of that Shahzad! Really nice write up, and explains some of the stuff I saw when typing the Doc stuff too. Really had no idea this language feature existed in Go.
I think I also agree with you and prefer the untyped version too by default, and should only type it when we have good reason to lock it down (for example for context keys perhaps).
There was a problem hiding this comment.
Great! Appreciate the discussion @fredcarle @AndrewSisley
It'd be nice to indicate the decoupling in the PR/commit title |
If there is a followup PR dedicated to the 'decoupling', then it'd be fine to just detail it in the commit message in this one. |
shahzadlone
force-pushed
the
lone/tools/add-goconst-linter
branch
from
34ec25a
to
9c7771b
Compare
shahzadlone
force-pushed
the
lone/tools/add-goconst-linter
branch
from
9c7771b
to
efdf778
Compare
- RELATED ISSUE(S): Resolves sourcenetwork#403 - DESCRIPTION: Enables the goconst linter and and resolves all it's errors. This was one of the remaining linters we didn't have enabled from the list of recommended ones that were mentioned in the book 100 go mistakes and how to avoid them. One side affect of this PR was that schema package was decoupled from using the parser package.
RELATED ISSUE(S):
Resolves #403
DESCRIPTION:
Enables the
goconstlinter and and resolves all it's errors.This was one of the remaining linters we didn't have enabled from the list of recommended ones that were mentioned in the book
100 go mistakes and how to avoid them.One side affect of this PR was that
schemapackage was decoupled from using theparserpackage.