Skip to content
This repository was archived by the owner on Nov 20, 2024. It is now read-only.

Conversation

@pq
Copy link
Contributor

@pq pq commented Feb 16, 2021

More tidying up.

On balance I think this makes the codebase a lot better and surely more safe. There were a number of places where I was tempted to do deeper structural re-organization but I opted not to in the interest of keeping this more manageable. At some point I do think a full audit would be in order.

There are a few test utilities left after this but this should get lib under control.

Thanks in advance for muddling through!

/cc @bwilkerson

@google-cla google-cla bot added the cla: yes label Feb 16, 2021
@coveralls
Copy link

coveralls commented Feb 16, 2021

Coverage Status

Coverage decreased (-0.01%) to 96.603% when pulling 8099720 on nnbd_tidy into 7849ee5 on master.

@parlough
Copy link
Collaborator

parlough commented Feb 16, 2021

Something that is not super consistent with these changes and elsewhere in null safety migrations is the difference between code similar(but not limited) to the following:

var person = group.leader;
return person != null && person.isCool;

versus

var person = group.leader;
return person?.isCool == true;

I'm not sure which I prefer, but I wonder if there should be a suggestion as part of effective dart for this situation and if we should/could add a potential lint. I'm assuming the preference would be the first one, as while it is not as concise, it is a bit more clear in intentions. Not super related to this PR, but thought I should inquire about it. Perhaps @munificent has some opinions around it.

Copy link
Contributor

@bwilkerson bwilkerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of my comments are required, so take them or leave them.

@pq
Copy link
Contributor Author

pq commented Feb 16, 2021

@parlough: thanks for noticing this. I almost wrote a note about it. I admit I feel pretty uneasy about code like foo?.isBar == true but sometimes the alternative is even more cumbersome. I'm not sure if there's a once-size-fits-all solution but would be happy if there were one if only so I didn't have to hem and haw each time.

@a14n
Copy link
Contributor

a14n commented Feb 16, 2021

return person?.isCool == true;

The recommendation from Effective dart is:

return person?.isCool ?? false;

Do we have a lint for that?

@pq
Copy link
Contributor Author

pq commented Feb 16, 2021

@a14n: I had missed that entirely. I appreciate the rationale too. We should absolutely lint for it though IMO.

dart-lang/sdk#58324

@pq pq merged commit 6e4d9d7 into master Feb 16, 2021
@pq pq deleted the nnbd_tidy branch February 16, 2021 22:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Development

Successfully merging this pull request may close these issues.

6 participants