[xdg_directories] Migrate to null safety #250
Conversation
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
I started to review this, but it seems like you have just made everything optional without looking at the code to see what can meaningfully be null and what can't. Please do a full pass over all of your changes and look at the semantics of the functions and this implementations to see what should actually be null, rather than just adding optionality everywhere.
|
@yash1200 Are you still able to work on this? |
|
@bparrishMines Yes I'm |
|
@yash1200 It looks like you only have a couple comments from @stuartmorgan to address and fixing the formatting in |
|
@bparrishMines please take a look at the updates. |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
I'll do a full review later, but a quick comment on the naming discussion.
| Directory? get runtimeDir => _directoryFromEnvironment('XDG_RUNTIME_DIR'); | ||
|
|
||
| /// Gets the xdg user directory named by `dirName`. | ||
| /// |
There was a problem hiding this comment.
Based on a bug that came in recently, we should make getUserDirectory return Directory?, because I think we'll want to add logic to make it catch run failures (e.g., due to xdg-user-dir not being installed) and return null, and changing the return type later would be a breaking change.
| expect(xdg.configHome.path, equals(testPath('.test_config'))); | ||
| expect(xdg.dataHome.path, equals(testPath('.local/test_share'))); | ||
| expect(xdg.runtimeDir.path, equals(testPath('.local/test_runtime'))); | ||
| expect(xdg.runtimeDir!.path, equals(testPath('.local/test_runtime'))); |
There was a problem hiding this comment.
This ! is not correct; the API can return a null. You need to validate both that it's not null, and that it matches if it's not.
goderbauer
force-pushed
the
nnbd
branch
2 times, most recently
from
7df957a to
7a98c90
Compare
|
@yash1200 Can you convert this to a pull request against master (or if that's not possible, open a new version of it against master, referencing this earlier PR)? |
|
Yeah sure |
|
The new PR is here #275 |
Advanced book example: Improve and migrate to null safety
4 participants
This PR migrates
xdg_directoriespackage to null safety (Waiting formockitoto be in nnbd, it'll be there by the end of december dart-lang/mockito#298 (comment)).Fixes flutter/flutter#71506