refactor: Remove CollectionDefinition #3939
Conversation
27f5f3b to
e2c5ab6
Compare
e2c5ab6 to
7883fca
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3939 +/- ##
===========================================
- Coverage 73.07% 73.04% -0.03%
===========================================
Files 463 463
Lines 42068 41945 -123
===========================================
- Hits 30738 30636 -102
+ Misses 9299 9284 -15
+ Partials 2031 2025 -6
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.
🚀 New features to boost your workflow:
|
| "graphql_request": &GraphQLRequest{}, | ||
| "backup_config": &client.BackupConfig{}, | ||
| "collection": &client.CollectionVersion{}, | ||
| "collection_definition": &client.CollectionDefinition{}, |
There was a problem hiding this comment.
question: should above collection be changed to collection_version?
There was a problem hiding this comment.
I'm happy with collection. Long term I think I want to remove client.Collection anyway (replacing it with db funcs that take a CollectionVersion), but want to see what Keenan ends up doing with client.Collection (I think he is looking to remove a lot of it).
Do you have a strong preference for collection_version here? And if so why?
fredcarle
left a comment
There was a problem hiding this comment.
LGTM. Just one suggestion.
client/collection_cache.go
Outdated
| // in the given store. | ||
| // | ||
| // If the related definition is not found, or an error occurs, default and false will be returned. | ||
| func GetDefinitionFromStore( |
There was a problem hiding this comment.
suggestion: GetVersionFromStore or GetCollectionFromStore if you want to be consistant with the cache version of this function. This PR is removing CollectionDefinition so it's weird to see this added with that name.
There was a problem hiding this comment.
Nice spot, I thought I changed this :)
- Rename GetDefinitionFromStore
shahzadlone
left a comment
There was a problem hiding this comment.
Great work sorting this out.
LGTM
Relevant issue(s)
Resolves #3845
Description
Removes
CollectionDefinition.Anything public that used to return it now returns
CollectionVersion.