refactor: Remove some possible panics from codebase #732
Conversation
numDocs property adds no value, and adds a new potential source of failure (if it becomes out of sync with the actual slice length). Panics add no value as the panic that would otherwise happen (index out of bounds) is plenty clear enough anyway. Returning error is not desired here as this struct is esentially an extension of a slice, and like slices index validation should be handled by callers - allowing them to determine whether it is safe or not to access elements without additional validation.
AndrewSisley
added
area/errors
| @@ -196,8 +196,8 @@ func newAllSortStrategy(v *valuesNode) *allSortStrategy { | |||
|
|
|||
| // Add adds a new document to underlying valueNode | |||
| func (s *allSortStrategy) Add(doc core.Doc) error { | |||
There was a problem hiding this comment.
suggestion:
| func (s *allSortStrategy) Add(doc core.Doc) error { | |
| func (s *allSortStrategy) Add(doc core.Doc) { |
| func (s *allSortStrategy) Add(doc core.Doc) error { | ||
| err := s.valueNode.docs.AddDoc(doc) | ||
| return err | ||
| s.valueNode.docs.AddDoc(doc) | ||
| return nil |
There was a problem hiding this comment.
suggestion: don't need to return nil if remove error from signature (obv also need to update orderingStrategy interface signature and remove a check from orderNode.Next() if we do this.
There was a problem hiding this comment.
I wanted to keep the error on the interface in case future implementations wanted to return an err. Having written that out it seems very hypocritical of me, but TBH I don't think this really matters. Give me a shout if you have a strong enough preference for removal for me to bother.
Public function signature changes are breaking changes and do not belong in minor releases. |
AndrewSisley
force-pushed
the
sisley/refactor/I615-panic-reduction
branch
from
e95fdfb to
b979f57
Compare
Codecov Report
@@ Coverage Diff @@
## develop #732 +/- ##
===========================================
+ Coverage 57.44% 57.48% +0.04%
===========================================
Files 143 143
Lines 16414 16390 -24
===========================================
- Hits 9429 9422 -7
+ Misses 6099 6085 -14
+ Partials 886 883 -3
|
The changes in this PR is fair game TBH. |
AndrewSisley
force-pushed
the
sisley/refactor/I615-panic-reduction
branch
from
b979f57 to
87a0f61
Compare
* Assert span implements Span * Remove dead span code * Handle db.PrintDump errors * Simplify doc container numDocs property adds no value, and adds a new potential source of failure (if it becomes out of sync with the actual slice length). Panics add no value as the panic that would otherwise happen (index out of bounds) is plenty clear enough anyway. Returning error is not desired here as this struct is esentially an extension of a slice, and like slices index validation should be handled by callers - allowing them to determine whether it is safe or not to access elements without additional validation. * Remove unused utils function
Relevant issue(s)
Part of #615
Description
Removes a handful of panics from our codebase. Avoids making breaking changes (see WARN commit - discussion required there as I do not know if this is public). Does not complete the issue as there are two panics listed within it that are not suitable for a patch release removing them would require a public function signature change.
TODO: