Skip to content

FileSystemEvent should be sealed #51912

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

Closed
3 tasks done
brianquinlan opened this issue Mar 30, 2023 · 2 comments
Closed
3 tasks done

FileSystemEvent should be sealed #51912

brianquinlan opened this issue Mar 30, 2023 · 2 comments
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team

Comments

@brianquinlan
Copy link
Contributor

brianquinlan commented Mar 30, 2023

This will allow developers to exhaustively pattern match on the Stream returned by FileSystemEntity.watch.

Unfortunately, none of the FileSystemEvent subclasses have public constructors so at least one package, package:watcher created it's own implementation to work around that.

So I think that we can solve this in 3 steps:

  • Make the FileSystemEvent subclasses final and provide public constructors.
  • Modify package:watcher (and possibly others) to use the public constructors instead of custom subclasses.
  • Make FileSystemEvent sealed
@brianquinlan brianquinlan added library-io P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team labels Mar 30, 2023
@brianquinlan brianquinlan self-assigned this Mar 30, 2023
@srawlins srawlins added the area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. label Mar 31, 2023
@dart-lang dart-lang deleted a comment from faizan003 Apr 2, 2023
@lrhn
Copy link
Member

lrhn commented Apr 3, 2023

Making FileSystemEvent sealed is breaking, now and in the future, but making it final today is non-breaking because there is no 3.0 code out there yet. So we should make FileSystemEvent itself final now, not just its subclasses.
That won't break watcher until it upgrades to 3.0, and there should be time enough to fix it before that.
And provide the public constructors now too, so people have a place to go when migrating.

(The alternative is to seal the FileSystemEvent class itself, but keep the subclasses implementable. Not sure there is any long-term benefit to that, but it's a possibility. But that still breaks watcher because it actually does implement FileSystemEvent on its abstract superclass, even if it doesn't need to.)

copybara-service bot pushed a commit that referenced this issue Apr 3, 2023
Bug: #51912
Change-Id: I05f7884e619e3014339f7642c1eeacc8b617155a
Tested: existing unit tests
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/292220
Reviewed-by: Martin Kustermann <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>
Reviewed-by: Sigmund Cherem <[email protected]>
Reviewed-by: Aske Simon Christensen <[email protected]>
Commit-Queue: Brian Quinlan <[email protected]>
Reviewed-by: Leaf Petersen <[email protected]>
@brianquinlan
Copy link
Contributor Author

I made FileSystemEvent and its subclasses final.

copybara-service bot pushed a commit that referenced this issue May 22, 2023
Bug: #52027, #51912
Change-Id: I154ffe5901e4248f48400e6c84568c9fe6dbcd83
CoreLibraryReviewExempt: aske on holiday
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/302452
Reviewed-by: Sigmund Cherem <[email protected]>
Commit-Queue: Brian Quinlan <[email protected]>
Reviewed-by: Lasse Nielsen <[email protected]>
Reviewed-by: Siva Annamalai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team
Projects
None yet
Development

No branches or pull requests

3 participants