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 GetPrimaryIndexDocKey from collection interface #279

Merged

Conversation

AndrewSisley
Copy link
Contributor

Part of #200

Spotted this as quick and (hopefully) conflict-free item that could be done in parallel with the other open PRs.

@AndrewSisley AndrewSisley added area/collections Related to the collections system refactor This issue specific to or requires *notable* refactoring of existing codebases and components labels Mar 7, 2022
@AndrewSisley AndrewSisley self-assigned this Mar 7, 2022
net/process.go Outdated
Comment on lines 109 to 115
func getPrimaryIndexDocKey(c client.Collection, key core.DataStoreKey) core.DataStoreKey {
return core.DataStoreKey{
CollectionId: fmt.Sprint(c.ID()),
IndexId: fmt.Sprint(c.PrimaryIndex().ID),
}.WithInstanceInfo(key)
}

Copy link
Member

Choose a reason for hiding this comment

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

same discussion form #277. These should ideally just be kept in the base package, and we can just call col.Description().GetXYZ()

Copy link
Contributor Author

@AndrewSisley AndrewSisley Mar 8, 2022

Choose a reason for hiding this comment

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

Discussed over zoom.

  • Move to (internally) shared location

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved onto description for now

No real reason for this to be on here, and it exposes internal types
@AndrewSisley AndrewSisley force-pushed the sisley/refactor/I200-client-vs-core-primary-dockey branch from 0b45ba4 to 907cee5 Compare March 10, 2022 17:02
@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #279 (907cee5) into develop (c13c31a) will decrease coverage by 0.01%.
The diff coverage is 37.50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #279      +/-   ##
===========================================
- Coverage    62.37%   62.36%   -0.02%     
===========================================
  Files           84       84              
  Lines         9245     9250       +5     
===========================================
+ Hits          5767     5769       +2     
- Misses        2871     2874       +3     
  Partials       607      607              
Impacted Files Coverage Δ
db/collection.go 53.43% <ø> (+0.19%) ⬆️
db/fetcher/versioned.go 49.18% <ø> (ø)
db/base/descriptions.go 81.81% <37.50%> (-7.77%) ⬇️

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.

LGTM

@AndrewSisley AndrewSisley merged commit 0faf80f into develop Mar 10, 2022
@AndrewSisley AndrewSisley deleted the sisley/refactor/I200-client-vs-core-primary-dockey branch March 10, 2022 20:08
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…rcenetwork#279)

* Remove GetPrimaryIndexDocKey from collection interface

No real reason for this to be on here, and it exposes internal types

* Remove commented out code
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 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