refactor: Remove dead code from client package and document remaining (part 1 of many) #356
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We should declare all our public errors here
No good declaring it internally if it is publicly accessable
Codecov Report
@@ Coverage Diff @@
## develop #356 +/- ##
===========================================
+ Coverage 64.92% 64.98% +0.06%
===========================================
Files 80 80
Lines 9194 9185 -9
===========================================
Hits 5969 5969
+ Misses 2613 2604 -9
Partials 612 612
|
|
I applaud the idea of making It is possibly useful to make the key and namespace constants public again later. It would be good to document John's reasoning of why the options like |
|
Might be more indicative to have the commit as |
AndrewSisley
changed the title
Changed |
I would have to question the merit of documenting why something that was never used was removed (other than the fact that it was unused) |
I mean as comment in this PR, if there was a reason such as a planned feature set, but otherwise I agree |
AndrewSisley
force-pushed
the
sisley/docs/I355-client-godocs
branch
2 times, most recently
from
d645083
to
bbcc440
Compare
I cant spot anywhere where we would want users to provide one of these, and don't want to encourage it
core.go is ambiguous with the newer types added to the package, and this file will get hard to read once documention expands the line count
Having moved the DB interface out, this name makes sense
This function is unlikely to be implemented for quite a while and as it modifies an existing collection, it comes with a load of synchronization questions that should be answered properly before we can assume it will exist in the currently declared form
Note: this function is untested
AndrewSisley
force-pushed
the
sisley/docs/I355-client-godocs
branch
from
bbcc440
to
2140f4d
Compare
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
shahzadlone
pushed a commit
to shahzadlone/defradb
that referenced
this pull request
Feb 23, 2024
…sourcenetwork#356) * Remove unneeded no-lint comment * Extract errors to own file We should declare all our public errors here * Declare IndexNotFound error * Declare ErrDocumentNotFound error No good declaring it internally if it is publicly accessable * Declare ErrInvalidUpdateTarget error * Declare ErrInvalidUpdater error * Declare ErrInvalidDeleteTarget error * Make dockey version constant private * Make namespaceSDNDocKeyV0 private * Make versionToNamespace private * Remove Undef DocKey I cant spot anywhere where we would want users to provide one of these, and don't want to encourage it * Remove unused CreateOpt struct * Remove unused UpdateOpt struct * Remove unused DeleteOpt struct * Move DB interface to own file core.go is ambiguous with the newer types added to the package, and this file will get hard to read once documention expands the line count * Rename core.go to collection.go Having moved the DB interface out, this name makes sense * Remove unimplemented CreateIndex function from public interface This function is unlikely to be implemented for quite a while and as it modifies an existing collection, it comes with a load of synchronization questions that should be answered properly before we can assume it will exist in the currently declared form * Return result from UpdateWith Note: this function is untested * Return result from DeleteWith * Add package level documentation * Document CType consts * Document errors * Document Collection interface * Document DocKeysResult * Document UpdateResult * Document DeleteResult * Tweak error messages
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #355
Was a documentation ticket, but I also removed some unwanted items (instead of having to document them). Put up for review now but might grow if the current questions around aggregates are not resolved before I resume work tomorrow morning.
Commits should be clean, might be easier to review commit by commit - some of the code changes are explained by the commit messages. A lot of the documentation of Collection members was copy-pasted+adapted from the concrete type in the db package.