refactor: Breakup core/keys.go file #3198
Conversation
AndrewSisley
added
area/db-system
AndrewSisley
changed the title
AndrewSisley
changed the title
Original file had grown way to large and was getting quite painful to find stuff in it, and I need to add new types to it shortly.
AndrewSisley
force-pushed
the
3197-brk-up-keys
branch
from
1028cc9 to
5852067
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3198 +/- ##
===========================================
- Coverage 77.52% 77.36% -0.15%
===========================================
Files 357 376 +19
Lines 34801 34807 +6
===========================================
- Hits 26976 26928 -48
- Misses 6216 6254 +38
- Partials 1609 1625 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| // by the Apache License, Version 2.0, included in the file | ||
| // licenses/APL.txt. | ||
|
|
||
| package keys |
There was a problem hiding this comment.
todo: Please use the singular for package name. key
There was a problem hiding this comment.
why? Go has lots of packages with plural names (e.g. strings) and there is nothing in https://go.dev/blog/package-names against it. keys contains many different key-types, and according to the norms I have experienced, the name should be plural.
There was a problem hiding this comment.
The packages in Go that use plural names are usually plural because the singular form is a reserved word or keyword (strings, types, errors, bytes).
It is generally preferred in the Go community to use singular names for packages. We've discussed this plenty of times before.
There was a problem hiding this comment.
Discussed over discord, and noted here for future reference:
When attempting to rename to key, I found this caused a large amount (+100) of variable shadowing (most places using this package want variables with the name key). Even if consensus is reached RE avoiding plural package names, we probably still want this to be called keys anyway (quite possibly the same reason why many std lib packages have plural names).
| ) | ||
|
|
||
| // DataStoreKey is a type that represents a key in the database. | ||
| type DataStoreKey struct { |
There was a problem hiding this comment.
suggestion: Since the package name is keys (should be key), the Key suffix is less relevant. I would suggest removing it to get key.DataStore. This applies for all keys.
There was a problem hiding this comment.
Yes, I agree, but I didn't want to bother dealing with the many niggles like this already present in the code I moved. I just want to break it up so I can add new stuff in a clean way.
There was a problem hiding this comment.
We've been doing this often lately — refactoring but not cleaning things up fully. It feels like a bad habit that's setting in that might hurt us in the future. This one is minor but still part of the habit so I'm pointing it out.
There was a problem hiding this comment.
Code is never cleaned up fully. Leaving as is.
There was a problem hiding this comment.
We've been doing this often lately — refactoring but not cleaning things up fully. It feels like a bad habit that's setting in that might hurt us in the future.
I am curious to know where we have done this lately? We should def have issues to track any pending cleanups if there were any.
Relevant issue(s)
Resolves #3197
Description
Extracts core/keys.go to multiple files.
Original file had grown way to large and was getting quite painful to find stuff in it, and I need to add new types to it shortly.