nullsafe migration

[robrbecker]

I ran through the null safety migration tool and generally accepted it's output. I wanted to get this branch up for testing.
Sorry it's such a big PR!

Give it a try to see how it works for you and report any issues here. Thanks!

[robrbecker]

FYI @pq @kevmoo Published this as a https://pub.dev/packages/github/versions/7.1.0-nullsafe.0

[pq]

I haven't dug deeply but I'm curious if in practice this is ever null?

[robrbecker]

It appears that it isn't null as far as the responses from github which are JSON deserialized into this object. This brings up another good point though .. how much should I go through the models and make as much non nullable as possible based on the github API?
What's currently in this branch is basically the minimum to meet the 2.12 null safe rules. It's very similar code as before which means things aren't likely to break since they were nullable before and there are null checks still in place.
The downside is tightening things up later would mean another major.

[kevmoo]

Ideally we'd be more strict. If we "know" something is not going to be null, we should type it as such.

Otherwise usability is going to be miserable!

[kevmoo]

easy for me to say, obviously. It's a lot of work to go through things and verify

[pq]

It's a balance. I know a lot of folks are using this as an opportunity to tighten as much as they can (since it's a breaking change release already). On the other hand you need to be practical and it's certainly better (for me anyway) to get something out there and iterate. The workarounds for me have not been terrible but are a little awkward. It'd be great to prevent other folks from needing to do the same dance. (How's that for a non-answer! 😬 )

[pq]

easy for me to say, obviously. It's a lot of work to go through things and verify

💯

and so we need to be practical.

[pq]

Thanks! I grabbed this (dart-lang/linter#2465) and so far so good w/ testing. It'd be cool if tag names were sure not be null but that's easy to work-around.

[robrbecker]

@pq Is this needed to be able to release a stable version of linter? And if so by when?

[kevmoo]

We can always do a follow-up breaking change that tighten things

[pq]

For the linter push, I think actually the current 7.1.0-nullsafe.0 is sufficient. As @kevmoo said, any desired tightening up can go into a follow-on breaking change release.

Thanks so much @robrbecker!

[robrbecker]

Excellent, if you can work with the current prerelease for now that gives me some more time to tighten up the API to actually be more null safe.

[pq]

Thanks a million!

[robrbecker]

@pq Looks like the 8.0.0-nullsafe.1 release is working for you? dart-lang/linter#2497
Did you have to work around any problems or need any more fixes?

[pq]

Thanks for checking in! I'm actually seeing what appears to be a hang when I try and fetch tags

https://github.com/dart-lang/linter/blob/e9296247ab2117cf3c0012888cd24baddc5888f8/tool/crawl.dart#L187

(Running dart tool/scorecard.dart.)

I'm pretty sure this was working in the previous version but haven't had time to confirm. Sorry of this is a red herring!

[robrbecker]

Hmm I made this little script with some of the relevant linter code copied and it seems to work. Without the exit it takes a while for the isolate to end, but it does eventually. Maybe that's the hang?

import 'dart:async';
import 'dart:io';
import 'package:github/github.dart';

Future main() async {
  var issues = await _getIssues();
  var bugs = issues.where(_isBug).toList();
  print(bugs);
  exit(0);
}

bool _isBug(Issue issue) => issue.labels.map((l) => l.name).contains('bug');


  Future<List<Issue>> _getIssues() async {
    var github = GitHub(auth: findAuthenticationFromEnvironment());
    var slug = RepositorySlug('dart-lang', 'linter');
    try {
      return github.issues.listByRepo(slug).toList();
    } on Exception catch (e) {
      print('exception caught fetching github issues');
      print(e);
      print('(defaulting to an empty list)');
      return Future.value(<Issue>[]);
    }
  }

[pq]

Great! Thanks for digging in. I'm not hugely surprised that this is bogging down as we have so many tags now; glad to see it's just a matter of patience. For what it's worth, I've known for a long time I needed to rethink how I'm calculating and caching since info and this is a great nudge to do so. Sorry for the noise!

Opened dart-lang/sdk#58342 to track a re-think on our end.