-
Notifications
You must be signed in to change notification settings - Fork 242
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
fix identity map #470
fix identity map #470
Conversation
Codecov Report
@@ Coverage Diff @@
## master #470 +/- ##
===========================================
- Coverage 98.8% 67.52% -31.29%
===========================================
Files 27 28 +1
Lines 1336 2069 +733
===========================================
+ Hits 1320 1397 +77
- Misses 16 672 +656
Continue to review full report at Codecov.
|
test/test_list.jl
Outdated
@@ -103,4 +103,10 @@ | |||
@test collect(l10) == [2; 4; 5.6; 10.5] | |||
end | |||
|
|||
@testset "l11" begin |
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.
Please give this @testset
a real name.
The fact that the existing testsets do not have real names is not a convention of this package.
It is a flaw resulting from the package being older than testsets.
And noone having gotten around to renaming things with meaninful names yet
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 see. Fixed.
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 actually spent a while thinking about this.
I concluded that it will do.
There is probably something smarter we can
do but this is definitely an improvement.
If you can fix the testset name we can merge
Yeah, I was thinking to have a more elegant solution for this, e.g keep the interface, but implement several different types (for different situation), like mutable, immutable, etc. But this is just a straight forward fix to make things work first. |
Something like what @andyferris did here for determining and progressively widening the type (I just have that PR handly for |
MWE: when there is a
LinkedList{Any}
, the following will error:This PR will fix it, and will allow more generic (each node has different generic types) linked list.