-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Improve error messages for shape and vec -like types #429
Improve error messages for shape and vec -like types #429
Conversation
Pull Request Test Coverage Report for Build 6666122755
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm +1 for this change!
To me, It is (at first feeling acceptibally) slower but at least it will give you helpful error messages.
Not sure how other people see this.
Some additional comments on the code:
- You only cover
coerce()
. We could have this onassert()
as well? - You currently don't cover paths for the types:
map
,mutable_map
,vector
,mutable_vector
,non_empty_vec
,non_empty_dict
,mixed_dict
,mixed_vec
$result = []; | ||
|
||
/** | ||
* @var Tk $k | ||
* @var Tv $v | ||
*/ | ||
foreach ($value as $k => $v) { | ||
$value_type = $this->value_type->withTrace( | ||
$trace->withFrameAtPath( | ||
'dict<_, ' . $this->value_type->toString() . '>', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string concatenations are fast, but maybe it helps to build the frame name outside of the loop and use it instead of always recalculating the frame name?
It will always be the same value.
/** @var Type\Type<Tv> $value_type */ | ||
$value_type = $this->value_type->withTrace( | ||
$this->getTrace() | ||
->withFrameAtPath($this->toString(), (string) $i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here It might help to build the frame name outside of the loop.
@@ -94,4 +94,29 @@ public function testConversionFailure(): void | |||
static::assertCount(0, $frames); | |||
} | |||
} | |||
|
|||
public function testCoercionPath(): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a start, but we'll need tests for every changed type individually.
This might become a bit too slow on big data structures. This issue needs some more thoughts. I'm gonna keep it open for now. |
198e166
to
d42f3fa
Compare
Given:
Results in:
The changed I made do have quite an impact on performance, but tried to keep it at a minimum. Mainly vecs and dicts are suffering. Not sure what the accepted range is.
The impact on shapes are minimal:
(See #412)