refactor: Move public members out of core and base packages #295
Conversation
AndrewSisley
added
the
refactor
There was a problem hiding this comment.
After sitting with it, im a big fan of this refactor. Especially the Descriptions and Document/Key stuff.
Few items I suggested changing. Some non-descript type names that are now defined in client (since its a general package).
Suggest moving EncodedDoc from fetcher into a new package encoding.
Im still reserved on the inclusion of CType in client, as I feel its reversing the depedancy direcetion that I think makes sense to maintain. The options are: move it to core (but you have reservations about this regarding the rest of the package being public), make it its own package (which kinda feels absurd since the entire package would be about 10 lines xD), or leave it where it is. Lmk your thoughts on this one.
Prob a few random other comments too. But all in all, very good!
| const ( | ||
| //no lint | ||
| NONE_CRDT = CType(iota) // reserved none type | ||
| LWW_REGISTER | ||
| OBJECT | ||
| COMPOSITE | ||
| ) |
There was a problem hiding this comment.
I see the reverse mapping enum was removed. I know its not being used, but lets keep it in as supporting utility, and can be helpful to have. If its not there now, I worry if another dev might implement a one-off approach.
There was a problem hiding this comment.
I disagree lol, but I'll put it back. Is this the current idiomatic golang way of expressing it? Would the below not be better, given if you wanted to convert a ByteToType you'd just do CType(aByte)?
ValidCTypes := map[CType]struct{}{...}
There was a problem hiding this comment.
CType(aByte) will create a new variable and theres no checks in place to enforce it follows the enum, it would be an easy way to shoot ourselves in the foot if we accidentally or unknowling intentionaly wrote CType(0x04) for example, as 0x04 isnt a valid option in the enum.
Having the reverse-map in place means we can do ByteToCType[0x04] and this will ensure we are consistent within our defined enums. The problem is that go doens't natively support enums, so the idiomatic pattern is to add some of these utilities to give us some level of (type) safety.
ValidCTypes := map[CType]struct{}{...} is really just the exact same process, either you use a map to get the correct CType for that Byte, or you convert the byte to a CType then check thats its valid. With the reverse map you get that in one go.
But again, they are basically the same, reverse-map is a common enum pattern in Go.
There was a problem hiding this comment.
Not too fussed about what we will do, but what our users will - and they can quite happily do CType(aByte) (this is the norm in some other langs, and the go compiler doesn't care either - which means we need to handle non-standard values anyway). ValidCTypes := map[CType]struct{}{...} supports checking of the type we care about (CType), and doesn't lock users into working with a type we dont care about (byte).
But again, they are basically the same, reverse-map is a common enum pattern in Go.
I guess that means it is idiomatic in go? Is ByteToCType the common naming format here - it feels quite verb-like?
There was a problem hiding this comment.
I'm not sure I'm following the thought here. Both implementations start from having a byte reference, either CType(byte) then verify, or ByteToCtype[byte]. The end result is the same, no?
There was a problem hiding this comment.
CType(byte) is client code - we dont worry about it - if using ByteToCtype[byte] we are forcing the validation via byte, even if the user already has a CType instance that they want to validate (which could have been instantiated without them dealing with the underlying byte - e.g. from json, or hidden from them from within a func).
And regardless of us providing a map - any of our code that handles an externally provided CType instance needs to handle 'non-standard' values, as the go compiler doesn't stop them from sending them in.
client/descriptions.go
Outdated
| type Meta uint8 | ||
|
|
||
| // Note: These values are serialized and persisted in the database, avoid modifying existing values | ||
| const ( | ||
| Meta_Relation_ONE Meta = 1 // 0b0000 0001 | ||
| Meta_Relation_MANY Meta = 2 // 0b0000 0010 | ||
| Meta_Relation_ONEONE Meta = 4 // 0b0000 0100 | ||
| Meta_Relation_ONEMANY Meta = 8 // 0b0000 1000 | ||
| Meta_Relation_MANYMANY Meta = 16 // 0b0001 0000 | ||
| _ Meta = 32 // 0b0010 0000 | ||
| Meta_Relation_INTERNAL_ID Meta = 64 // 0b0100 0000 | ||
| Meta_Relation_Primary Meta = 128 // 0b1000 0000 Primary reference entity on relation | ||
| ) |
There was a problem hiding this comment.
Meta is now non-descript since being moved from base to client. Tbh, it wasn't very descript before when it lived in base but that package was more narrowly scoped compared to client, so the problem was just worsend.
Suggest FieldMeta or something :)
There was a problem hiding this comment.
FieldRelationMeta? 😂. Both work, ill let you choose.
There was a problem hiding this comment.
lol no, just Relation, or maybe RelationType - no meta
Is used by all the client packages
And make private
AndrewSisley
force-pushed
the
sisley/refactor/I200-mv-descriptions
branch
from
ad13ec7
to
cdaa962
Compare
AndrewSisley
force-pushed
the
sisley/refactor/I200-mv-descriptions
branch
2 times, most recently
from
5fb3a11
to
e563857
Compare
Codecov Report
@@ Coverage Diff @@
## develop #295 +/- ##
===========================================
+ Coverage 64.69% 65.07% +0.38%
===========================================
Files 81 80 -1
Lines 9035 8979 -56
===========================================
- Hits 5845 5843 -2
+ Misses 2574 2520 -54
Partials 616 616
|
AndrewSisley
force-pushed
the
sisley/refactor/I200-mv-descriptions
branch
from
e563857
to
2c0fc72
Compare
…twork#295) * Use exisiting pub func instead of private * Rename maker file * Move key utils from descriptions file to col_keys file * Remove unused variables * Remove unused byteToType var * Remove comment out code * Move CType into client Is used by all the client packages * Move store namespaces into datastore package * Delete simple document * Move document and descriptions into client package * Move encoded doc into fetcher And make private * Remove unused value types * Give type to Meta field * Make key utils take value instead of pointer PR request
Part of #200
Moves descriptions out of base and into client. Also moves (most of) document, key and CType into client - partly to solve circular dependency issues. I think document, key and ctype could be moved into a sub-package if we want (now that encoded doc is in fetcher), but they need to be done at the same time.
Also made some further, smaller tweaks to reduce the surface area - there is more we can do, but I think after this and the other (linked) PR nothing major remains and I can get back to feature work. Suggest reviewing commit-by-commit (is one very large commit though, sorry).
Todo: