-
Notifications
You must be signed in to change notification settings - Fork 29
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
Enable strictNullChecks #77
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #77 +/- ##
==========================================
+ Coverage 96.22% 96.24% +0.02%
==========================================
Files 13 13
Lines 1508 1517 +9
Branches 136 146 +10
==========================================
+ Hits 1451 1460 +9
Misses 57 57
Continue to review full report at Codecov.
|
[Symbol.iterator]: generator | ||
}; | ||
}; | ||
|
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.
Instead of this change, what about implementing the linked list as an abstract class called List
which Cons
and Nil
extends? Then the iterator and the isNil
property could sit on the abstract class. I also think that would fit better with the rest of the code where we use abstract classes and classes that extend them to represent something like algebraic data types/sum types.
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.
Thanks for the reply. Personally I just use tagged unions and simple constructors to encode ADT in TypeScript because it lets to check such unions exhaustively, sort of exhaustive pattern matching. But I'll check your suggestion, thanks!
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.
@paldepind Looks like it's impossible to declare Cons
using abstract class, because it would require declaring value
and tail
fields on Nil
with some "nully" values to make shared generator work. That would defeat the very purpose of this PR - get rid of such values.
Declaring Cons
using tagged union OTOH allows to declare value
and tail
only when isNil === false
(means using isNil
as tag).
Also I don't get why |
7ab076d
to
009c49e
Compare
Does it make sense to continue on this? Is the only reason it is not merged yet the 0.12% missing test coverage of the patch? In case, I suggest to append this commit, covering some additional lines: DSoko2@163328b |
Hi, this PR is a continuation of #76 (fully includes that changes, so #76 should be merged first). This PR enables TS strictNullChecks flag and contains corresponding changes.
There're no breaking changes.