Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Move public members out of core and base packages #295

Merged
merged 14 commits into from
Mar 17, 2022

Conversation

AndrewSisley
Copy link
Contributor

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).

image

Todo:

@AndrewSisley AndrewSisley added the refactor This issue specific to or requires *notable* refactoring of existing codebases and components label Mar 14, 2022
@AndrewSisley AndrewSisley self-assigned this Mar 14, 2022
Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Comment on lines +16 to 22
const (
//no lint
NONE_CRDT = CType(iota) // reserved none type
LWW_REGISTER
OBJECT
COMPOSITE
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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{}{...}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Resolve this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Mar 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

core/type.go Show resolved Hide resolved
db/base/collection_keys.go Show resolved Hide resolved
client/value.go Show resolved Hide resolved
client/value.go Outdated Show resolved Hide resolved
db/fetcher/encoded_doc.go Show resolved Hide resolved
merkle/crdt/composite.go Show resolved Hide resolved
db/base/collection_keys.go Show resolved Hide resolved
Comment on lines 134 to 146
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
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FieldRelationMeta? 😂. Both work, ill let you choose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol no, just Relation, or maybe RelationType - no meta

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RelationType works for me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Rename Meta

Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving now since you have all the context needed to finalize, and I don't want you waiting on me to approve after the changes are made.

@AndrewSisley AndrewSisley force-pushed the sisley/refactor/I200-mv-descriptions branch from ad13ec7 to cdaa962 Compare March 17, 2022 13:31
@AndrewSisley AndrewSisley force-pushed the sisley/refactor/I200-mv-descriptions branch 2 times, most recently from 5fb3a11 to e563857 Compare March 17, 2022 13:40
@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #295 (2c0fc72) into develop (8951302) will increase coverage by 0.38%.
The diff coverage is 86.39%.

Impacted file tree graph

@@             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              
Impacted Files Coverage Δ
merkle/crdt/merklecrdt.go 26.08% <ø> (ø)
query/graphql/planner/versionedscan.go 0.00% <ø> (ø)
db/collection_delete.go 60.08% <33.33%> (+1.95%) ⬆️
db/fetcher/versioned.go 49.18% <60.00%> (ø)
db/collection_update.go 42.89% <70.00%> (+0.78%) ⬆️
query/graphql/schema/relations.go 62.37% <72.72%> (ø)
db/collection.go 53.43% <80.55%> (ø)
client/descriptions.go 90.00% <90.00%> (ø)
query/graphql/schema/descriptions.go 86.13% <92.00%> (ø)
db/base/collection_keys.go 93.10% <93.10%> (ø)
... and 22 more

@AndrewSisley AndrewSisley force-pushed the sisley/refactor/I200-mv-descriptions branch from e563857 to 2c0fc72 Compare March 17, 2022 13:43
@AndrewSisley AndrewSisley merged commit d40a3cc into develop Mar 17, 2022
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor This issue specific to or requires *notable* refactoring of existing codebases and components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants