null safety #52
null safety #52
Conversation
|
"Some checks haven’t completed yet" - because of dart 2.2 instead of dart 2.12 |
|
@hovadur Are you still working on this? If you want, you can give me write access to your fork and I'll fix the CI and the conflicts. |
I'll fix it today |
* Fix List deprecation * Support old sdks
All checks have passed :) |
|
@kevmoo Sorry to bother you here, but could you please review and publish this if possible? This is blocking |
lib/src/crawl_async.dart
Outdated
| @@ -34,7 +34,7 @@ final _empty = Future<Null>.value(null); | |||
| /// have not completed. If the [edges] callback needs to be limited or throttled | |||
| /// that must be done by wrapping it before calling [crawlAsync]. | |||
| Stream<V> crawlAsync<K, V>(Iterable<K> roots, FutureOr<V> Function(K) readNode, | |||
| FutureOr<Iterable<K>> Function(K, V) edges) { | |||
| FutureOr<Iterable<K>?> Function(K, V) edges) { | |||
There was a problem hiding this comment.
@natebosch I am a bit surprised we allow returning null here? should we retain that behavior?
There was a problem hiding this comment.
No, I think we should restrict it - we previously allowed null and [] to mean the same thing, and there is no need to do that in a null safe world.
I'm on the fence about statically typing it as non-nullable, but dynamically allowing null to maintain backwards compatibility for opt-out code. Even with a major version bump it might be friendly to do so.
lib/src/shortest_path.dart
Outdated
| @@ -24,12 +24,12 @@ import 'dart:collection'; | |||
| /// | |||
| /// If you supply one of [equals] or [hashCode], you should generally also to | |||
| /// supply the other. | |||
| List<T> shortestPath<T>( | |||
| List<T>? shortestPath<T>( | |||
There was a problem hiding this comment.
Should this be <T extends Object>? I don't know that null is ever useful value here... but maybe we don't want to be overly opinionated about it. There is an assert below that start is not null though.
There was a problem hiding this comment.
My gut reaction is that most of these should be <T extends Object>, but I could also be convinced otherwise.
|
@hovadur - the closer I look at this the more I'm realizing there are some subtle choices to make that could have an outsized impact on usability and how the migration looks. Sorry for the delays in reviews so far, and for any churn. I'd like to take some more time and consider a couple different options for this migration against existing usage. In particular one thing I'd like to make sure we make the right decision on is whether the return type of the various In In My gut is that disallowing null return is the right thing to do for the null safety release, and I'm going to take a look at what the implications are for callsites in terms of usability and the effect on type inference in typical usage. |
done |
- Go to `1.0.0-dev`. - Add some `<Never>` on empty list literals to improve inference in some test usages. Code that migrates to null safety will likely need the same. - Use a few local variables to allow flow analysis. - Switch a `whereType` to a `!` - Add more `extends Object` to the generics that we use as map keys. No usage where a `null` is used would be likely to be correct. Retain nullable values allowe din `crawlAsync`, but don't write as `V?` since the `V` can be nullable in usage. - Remove comments about null arguments. - Handle the merge of the tweaked `shortestPath` - Clean up a stale lint violation and ignore. - Don't use nullable arguments for `equals` callbacks. `null` is only ever equal to `null`. - Remove tests around assertion errors for null arguments. - [Prior nit fix] Rename `getValue` to `readEdges`.
No description provided.