Skip to content

Refactor FileBasedAssetReader and FileBasedAssetWriter to ReaderWriter. #3876

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

Merged
merged 1 commit into from
Feb 26, 2025

Conversation

davidmorgan
Copy link
Contributor

@davidmorgan davidmorgan commented Feb 24, 2025

For #3811.

FileBasedAssetReader and FileBasedAssetWriter merge into ReaderWriter, and can now be used for testing too when an in-memory filesystem is provided, replacing most of InMemoryAssetReaderWriter. Finding assets is split out to classes InMemoryAssetFinder and PackageGraphAssetFinder to allow it to work for both cases.

Copy link

github-actions bot commented Feb 24, 2025

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

@davidmorgan davidmorgan force-pushed the reader-writer branch 6 times, most recently from 5256e8f to 8c772c4 Compare February 25, 2025 09:53
@davidmorgan davidmorgan requested a review from jensjoha February 25, 2025 10:06
@davidmorgan davidmorgan marked this pull request as ready for review February 25, 2025 10:06
required super.assetPathProvider,
required super.filesystem,
required super.cache,
required super.inputTracker,

Choose a reason for hiding this comment

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

You assume below this is non-null. Shouldn't you mark it as such?

Choose a reason for hiding this comment

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

(and possibly overwrite the inputTracker getter to non-null to avoid some !s below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's awkward because of the extends relationship, the super class copyWith wants it to be nullable. It could be covariant.

Really the awkward part is the test and non-test code doing different things, making it nullable for one and not for the other.

Fortunately the next PR addresses exactly this, unifying test and non-test use and moving InputTracker out of AssetReader entirely to the one place it's actually used :)

Thanks.

Choose a reason for hiding this comment

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

I don't think I understand... Why wouldn't this work:

diff --git a/build_test/lib/src/in_memory_reader_writer.dart b/build_test/lib/src/in_memory_reader_writer.dart
index f4ca7bdd..66e010f4 100644
--- a/build_test/lib/src/in_memory_reader_writer.dart
+++ b/build_test/lib/src/in_memory_reader_writer.dart
@@ -43,9 +43,12 @@ class InMemoryAssetReaderWriter extends ReaderWriter
     required super.assetPathProvider,
     required super.filesystem,
     required super.cache,
-    required super.inputTracker,
+    required InputTracker super.inputTracker,
   }) : super.using();
 
+  @override
+  InputTracker get inputTracker => super.inputTracker!;
+
   @override
   InMemoryAssetReaderWriter copyWith({
     AssetPathProvider? assetPathProvider,

I mean, with it going away it doesn't matter, but I don't understand the argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you are correct.

The problem I mentioned only happens if inputTracker is a parameter to copyWith, but it no longer is. Sorry for the confusion, I was repeating compiler complaints from a stale state :)

@davidmorgan davidmorgan merged commit c663785 into dart-lang:master Feb 26, 2025
76 checks passed
@davidmorgan davidmorgan deleted the reader-writer branch February 26, 2025 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants