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: Remove DataStoreKey from (public) dockey struct #278

Merged
merged 4 commits into from
Mar 7, 2022

Conversation

AndrewSisley
Copy link
Contributor

Closes #169
Part of #200 (broken out of #277 )

Removes DataStoreKey from dockey. dockey is part of the public abi and should not expose internal types (was also quite strange and a little circular). Public Collection functions still take dockey (which lacks internal stuff such as collectionId which has no reason to be a public param for these functions), internal/private functions take DataStoreKey allowing easier internal processing.

Is not used, and of non-public type
Was weird, circular, and exposed an internal type
@AndrewSisley AndrewSisley added area/collections Related to the collections system refactor This issue specific to or requires *notable* refactoring of existing codebases and components code quality Related to improving code quality labels Mar 7, 2022
@AndrewSisley AndrewSisley self-assigned this Mar 7, 2022
@codecov
Copy link

codecov bot commented Mar 7, 2022

Codecov Report

Merging #278 (f6bca07) into develop (f0bac01) will increase coverage by 0.15%.
The diff coverage is 91.11%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #278      +/-   ##
===========================================
+ Coverage    58.12%   58.28%   +0.15%     
===========================================
  Files          103      103              
  Lines        10224    10200      -24     
===========================================
+ Hits          5943     5945       +2     
+ Misses        3647     3622      -25     
+ Partials       634      633       -1     
Impacted Files Coverage Δ
document/field.go 66.66% <ø> (+5.55%) ⬆️
db/collection.go 53.23% <78.94%> (+0.24%) ⬆️
core/key.go 85.71% <100.00%> (-2.32%) ⬇️
db/collection_delete.go 58.13% <100.00%> (+1.39%) ⬆️
db/collection_get.go 51.02% <100.00%> (+1.02%) ⬆️
db/collection_update.go 42.10% <100.00%> (ø)
document/document.go 63.95% <100.00%> (ø)
document/key/dockey.go 51.16% <100.00%> (+16.49%) ⬆️

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.

Im a fan of the refactor.

We should prob have some utility functions to go from key.DocKey types to core.DataStoreKey so we're not always doing manual (error-prone) struct instanciation.

The helper func can really be placed anywhere. Either as a new constructor type on the core package, or as a method on the key.DocKey type. Up to you :)

@AndrewSisley
Copy link
Contributor Author

AndrewSisley commented Mar 7, 2022

Im a fan of the refactor.

We should prob have some utility functions to go from key.DocKey types to core.DataStoreKey so we're not always doing manual (error-prone) struct instanciation.

The helper func can really be placed anywhere. Either as a new constructor type on the core package, or as a method on the key.DocKey type. Up to you :)

Not a problem, will add (probably a new constructor)

  • add a core.DataStoreKey struct init helper taking a key.dockey

@AndrewSisley AndrewSisley requested a review from jsimnz March 7, 2022 22:22
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.

sweet, LGTM

@AndrewSisley AndrewSisley merged commit 2a32f05 into develop Mar 7, 2022
@AndrewSisley AndrewSisley deleted the sisley/refactor/I169-dockey-circle branch March 7, 2022 23:05
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…ork#278)

* Remove dockey.peerId and unused functions

* Remove key from field

Is not used, and of non-public type

* Remove DataStoreKey from dockey

Was weird, circular, and exposed an internal type

* Use constructor for DataStoreKey from dockey
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/collections Related to the collections system code quality Related to improving code quality 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.

dockey.go.DocKey is weird and circular and needs more work
2 participants