refactor: Cleanup Linter Complaints and Setup Makefile

[shahzadlone]

DESCRIPTION:

This PR sets up a Makefile workflow and resolves the cries from these following Go Linters (around 150 linter errors):
- deadcode
- gosimple
- govet
- ineffassign
- staticcheck
- structcheck
- typecheck
- unused
- varcheck

ISSUE:

Started from #42 but added alot more goodies.

BEFORE-MERGE:

Since a lot of dead code had to be edited out, I predict lots of breakage if new features are merged that make use of any of that code that was commented (without syncing with this branch). So really ensure even after there are no conflicts that nothing broke.
Needs a very thorough review to ensure I haven't done any BAD GO THINGS while resolving these linter errors.
Would prefer like @AndrewSisley suggested to also delete as much code as possible rather than having comments before the merge.

TODO:

There are tons more linters we can now very easily add with our configuration file (.golangci.sourceinc.yaml). Some good ones that I think we can add soon are:

• errcheck
• wrapcheck
• errorlint
• gci
• exportloopref
• megacheck
• govet
• wsl
• nilerr
• nilnil

NOTES:

There are some nolint:<linter-to-ignore> style placed to suppress the linter that we should eventually address. These can also be quickly found if you run make lint:todo command (and have ripgrep installed).

[todo]

Handle collection name from @collection directive

defradb/query/graphql/schema/generate.go

Lines 676 to 681 in be4c24b

	// // @todo: Handle collection name from @collection directive
	// Name: name,
	// Type: obj,
	// Args: gql.FieldConfigArgument{
	// "filter": newArgConfig(config.filter),
	// },

---

This comment was generated by todo based on a todo comment in be4c24b in #63. cc @sourcenetwork.

[AndrewSisley]

Looks good to me - thanks for the cleanup :) Just have a couple of minor comments on specific lines, plus the below:

1. I would much prefer the deletion of unused code instead of commenting it out, in most cases you seem to be making the problem worse in a sense by adding an an extra line of eye-space describing that it is commented out because it is unused.
2. Having to declare new names for every error variable within a scope doesn't seem ideal, and feels like a waste of mental energy when writing, and needless code clutter when reading - however this feels like it is potentially useful due to a Go 'quirk' allowing multiple variables with the same name (instead of the usual overwrite the existing variable in most langs I've worked with). Is this ever a problem with errors and we want to enforce the lint-rule, or is it safe to disable this one? (or is the lint rule getting me confused and Go behaves like a normal lang here, and the lint is about mutation or something?)

[shahzadlone]

Thanks for the review @AndrewSisley, responding to your comment here.

Looks good to me - thanks for the cleanup :) Just have a couple of minor comments on specific lines, plus the below:
I would much prefer the deletion of unused code instead of commenting it out, in most cases you seem to be making the problem worse in a sense by adding an an extra line of eye-space describing that it is commented out because it is unused.

I am on the same same page about deletion of unused code instead of commenting it out, I did that for now as It is a big batch of removal for lots of unused code spanning many files and there could perhaps be some important unused code that we may want for reference (sometimes it's easier to have some code as comments rather than to look at past git history). Will be nice if @jsimnz can point out code that I commented out that he would like to keep for reference, this way before the PR gets merged I can delete that code. Another reason I was hesitant to delete code straight away was if there was any partial implementation that was being worked on and somehow got merged into develop (which we should absolutely avoid in the future). The extra line of describing why the comment was there is only for the draft PR review to help understand why a comment was made, rest assured those can also be removed before the merge.

Having to declare new names for every error variable within a scope doesn't seem ideal, and feels like a waste of mental energy when writing, and needless code clutter when reading - however this feels like it is potentially useful due to a Go 'quirk' allowing multiple variables with the same name (instead of the usual overwrite the existing variable in most langs I've worked with). Is this ever a problem with errors and we want to enforce the lint-rule, or is it safe to disable this one? (or is the lint rule getting me confused and Go behaves like a normal lang here, and the lint is about mutation or something?)

Absolutely, I found that this was only an issue for 2 of the linters which now may have been disabled, it was referring to ineffectual assignment in GO if you are curious as to what it was. Another one had issues overwriting only when inside a nested block, which I am also guessing was due to some GO quirk. I changed and played with lots of different linters, I think the ones we have now shouldn't annoy us with issues like this anymore. If they do, let me know and we can have a chat and configure the linter rules again.

[AndrewSisley]

Ahh I wouldnt worry about partial implementations here - you are deleting full declarations, not parts of function so the compiler will flag anything pretty safely I think. But fair enough :) I can be quite heavy-handed with this kind of stuff lol

Nice - thanks for that effort plus info (re error stuff) :)

[jsimnz]

just a few changes.... :D

[todo]

paramaterize ns/suffix

defradb/core/crdt/base.go

Lines 39 to 42 in 8ccafae

	// @todo paramaterize ns/suffix
	func newBaseCRDT(store core.DSReaderWriter, namespace ds.Key) baseCRDT {
	return baseCRDT{
	store: store,

---

This comment was generated by todo based on a todo comment in 8ccafae in #63. cc @sourcenetwork.

[todo]

Expand delta metadata (investigate if needed)

defradb/core/crdt/lwwreg.go

Lines 37 to 40 in 8ccafae

	// @todo: Expand delta metadata (investigate if needed)
	type LWWRegDelta struct {
	Priority uint64
	Data []byte

---

This comment was generated by todo based on a todo comment in 8ccafae in #63. cc @sourcenetwork.

[todo]

check if NonNull is possible here

defradb/query/graphql/schema/generate.go

Lines 195 to 198 in 8ccafae

	// @todo: check if NonNull is possible here
	//case *gql.NonNull:
	// get subtype
	}

---

This comment was generated by todo based on a todo comment in 8ccafae in #63. cc @sourcenetwork.

[jsimnz]

lgtm

Got a lgtm