google / file.dart

A generic file system abstraction for Dart.
https://pub.dev/packages/file
122 stars 48 forks source link

Make MemoryFile.openRead and _ChrootFile.openRead return Stream<List<int>> again #217

Closed jamesderlin closed 1 year ago

jamesderlin commented 1 year ago

This is equivalent to https://github.com/google/file.dart/pull/168, but I foolishly neglected to make corresponding changes for all file system types.

Fixes https://github.com/google/file.dart/issues/193.

jamesderlin commented 1 year ago

This would be a breaking change so probably should wait for the next major version change (perhaps https://github.com/google/file.dart/pull/210).

jamesderlin commented 1 year ago

And based on the CI failures, bumping the major version apparently is a can of worms because file depends on test which depends on test_core which depends on glob which depends on file... How was this handled last time?

dnfield commented 1 year ago

Why does glob depend on file?

dnfield commented 1 year ago

@jakemac53 WDYT? Looks like you introduced the file dep on glob here: https://github.com/dart-lang/glob/commit/ba29990778bb2f8ad884cd4e5ab654cb766e3e87

jakemac53 commented 1 year ago

And based on the CI failures, bumping the major version apparently is a can of worms because file depends on test which depends on test_core which depends on glob which depends on file... How was this handled last time?

I believe you could fix this by overriding the dependency on glob:

dependency_overrides:
  glob: 2.1.1

(or if that doesn't fix it, some other combination of overrides will).

@jakemac53 WDYT? Looks like you introduced the file dep on glob here: dart-lang/glob@ba29990

Glob depends on file because it supports globbing file systems - and we generalized that support to arbitrary file systems using the package:file apis, which allowed us to drop the required dart:io dependency, and makes glob compatible with any platform.

jakemac53 commented 1 year ago

(note that the dependency override is only for a dev dependency so it isn't an issue, and once we publish a new package:glob that allows 7.x then it can be removed)

jakemac53 commented 1 year ago

Note that as a part of 7.x we should also do https://github.com/google/file.dart/issues/209, the pr is here https://github.com/google/file.dart/pull/210.

I believe the lower bound sdk constraint of >3.0.0 is also going to be a requirement in order to safely land this change.

jakemac53 commented 1 year ago

The other breaking change (migrate to Dart 3) has landed now.

jamesderlin commented 1 year ago

Hm, GitHub seems to be confused by me rebasing, resolving the conflicts, and force-pushing. I suppose I'll have to recreate the PR.

bernaferrari commented 1 year ago

What's the issue?

jamesderlin commented 1 year ago

GitHub didn't pick up my updated changes from force-pushing, but it looks like it managed to sort itself out eventually, so I don't need to recreate the PR after all.

This PR still needs a review though.

jamesderlin commented 1 year ago

Thanks for the reviews! (@jakemac53 or @natebosch, could I trouble either of you to take a look at the other package:file PRs too?)