Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2426773004:
First cut at a file system abstraction for the front end. (Closed)
|
Created:
4 years, 2 months ago by Paul Berry Modified:
4 years, 2 months ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionFirst cut at a file system abstraction for the front end.
R=brianwilkerson@google.com, scheglov@google.com
Committed: https://github.com/dart-lang/sdk/commit/8fca9ba9683047d7f96f4578a920f8f7c4c30640
Patch Set 1 #
Total comments: 22
Patch Set 2 : Address code review comments #
Total comments: 10
Patch Set 3 : Address additional code review comments #
Messages
Total messages: 10 (2 generated)
paulberry@google.com changed reviewers: + brianwilkerson@google.com, scheglov@google.com
Notes: - This is based on analyzer's ResourceProvider class, except that the File/Folder distinction has been dropped. - Unlike analyzer's ResourceProvider, the interface is asynchronous--this will allow the abstraction to be used in situations where a synchronous implementation is impossible (e.g. where file contents must be fetched via an RPC mechanism, as in Kythe). - I've deliberately made the API very narrow for now in order to reduce the risk of breaking changes in the future--it just contains the methods that I believe will be needed to implement the front end as it's been defined so far. If we discover the need for more methods, we can add them. - For now I've included the comment "Not intended to be implemented or extended by clients" on the abstract classes, so that if we add additional methods to them down the road, it won't constitute a breaking change. Once the API is well established, it should be safe to remove these comments and allow clients to provide their own implementations. https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... File pkg/front_end/lib/file_system.dart (right): https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:44: abstract class FileSystemEntity { I'm not sure how useful this class is. An alternative implementation would be to put all the functionality in FileSystem and just use paths, e.g.: abstract class FileSystem { path.Context get context; Future<List<int>> readFileBytes(String path); Future<String> readFileString(String path); } (Clients that wanted to convert between URIs and paths could still do so using FileSystem.context.fromUri() and FileSystem.context.toUri().) Thoughts?
https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... File pkg/front_end/lib/file_system.dart (right): https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:13: /// All front end interaction with the filesystem goes through this interface; "filesystem" or "file system" here? https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:29: FileSystemEntity entityForPath(String path); Maybe drop some extra strings and just name this File. https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:44: abstract class FileSystemEntity { On 2016/10/18 16:56:43, Paul Berry wrote: > I'm not sure how useful this class is. An alternative implementation would be > to put all the functionality in FileSystem and just use paths, e.g.: > > abstract class FileSystem { > path.Context get context; > Future<List<int>> readFileBytes(String path); > Future<String> readFileString(String path); > } > > (Clients that wanted to convert between URIs and paths could still do so using > FileSystem.context.fromUri() and FileSystem.context.toUri().) > > Thoughts? I like having a more structured API with separate object for separate file. https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:48: /// Returns a `file:` URI representing this file system entity. Is it a Uri wrapper for [path], or the original Uri from ""entityForUri? https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:57: Future<List<int>> readFileBytes(); Maybe just readBytes(). I don't think we need to make a distinction between file existence and reading error. Even if we perform a check (how - synchronously?), it is possible that the file will be removed before we are able to read it. https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:65: Future<String> readFileString(); Again, maybe just readString(). We could specify that the file is read as UTF8. The same comment about checking that file exists.
https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... File pkg/front_end/lib/file_system.dart (right): https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:20: /// Returns a [path.Context] suitable for use with this [FileSystem]. opinion: "[path.Context]" --> "context" | "path context". The type is on the next line (for navigation purposes) and this reads much better. https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:26: /// example, `entityForPath('./foo') and entityForPath('foo') are equivalent). nit: Missing or extra back-quote(s). https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:44: abstract class FileSystemEntity { I agree. 1. It's better to have objects with an API than to pass around primitive types like strings. 2. If you fail when passed invalid file paths, then you have one point of failure--when creating the instance--rather than a possible exception for every method you support. 3. You can provide a single readFileAsString() method rather than a readFileAsString(String) / readUriAsString(Uri) pair. https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:45: /// Returns the absolute path represented by this file system entity. Is it a normalized path or the path passed in to entityForPath? https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:57: Future<List<int>> readFileBytes(); > Maybe just readBytes(). Unless there's a strong argument to do otherwise I think we should match the protocol from dart:io so that authors of clients of the front end don't have to learn a new API. > I don't think we need to make a distinction between file existence and reading > error. Even if we perform a check (how - synchronously?), it is possible that > the file will be removed before we are able to read it. I agree. Any failure to provide the content should throw an exception. I believe that's the way the dart:io API works, and I think we should be consistent.
PTAL https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... File pkg/front_end/lib/file_system.dart (right): https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:13: /// All front end interaction with the filesystem goes through this interface; On 2016/10/18 17:10:32, scheglov wrote: > "filesystem" or "file system" here? I'm now using "file system" consistently. https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:20: /// Returns a [path.Context] suitable for use with this [FileSystem]. On 2016/10/18 17:36:59, Brian Wilkerson wrote: > opinion: "[path.Context]" --> "context" | "path context". The type is on the > next line (for navigation purposes) and this reads much better. Done. https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:26: /// example, `entityForPath('./foo') and entityForPath('foo') are equivalent). On 2016/10/18 17:36:59, Brian Wilkerson wrote: > nit: Missing or extra back-quote(s). Done. https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:29: FileSystemEntity entityForPath(String path); On 2016/10/18 17:10:32, scheglov wrote: > Maybe drop some extra strings and just name this File. I'm concerned that this would lead to confusion, because the thing pointed to by a FileSystemEntity might not be a file; it might be a folder (or might not exist). https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:44: abstract class FileSystemEntity { On 2016/10/18 17:36:59, Brian Wilkerson wrote: > I agree. > > 1. It's better to have objects with an API than to pass around primitive types > like strings. > > 2. If you fail when passed invalid file paths, then you have one point of > failure--when creating the instance--rather than a possible exception for every > method you support. > > 3. You can provide a single readFileAsString() method rather than a > readFileAsString(String) / readUriAsString(Uri) pair. Acknowledged. Leaving as is. https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:45: /// Returns the absolute path represented by this file system entity. On 2016/10/18 17:36:59, Brian Wilkerson wrote: > Is it a normalized path or the path passed in to entityForPath? The normalized path. I've added a clarifying comment. https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:48: /// Returns a `file:` URI representing this file system entity. On 2016/10/18 17:10:31, scheglov wrote: > Is it a Uri wrapper for [path], or the original Uri from ""entityForUri? It's a Uri built from [path] (it has to be because the original Uri passed to [entityForUri] might have contained `..` or `.` which got normalized away). I've added a clarifying comment. https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:57: Future<List<int>> readFileBytes(); On 2016/10/18 17:10:32, scheglov wrote: > Maybe just readBytes(). > > I don't think we need to make a distinction between file existence and reading > error. Even if we perform a check (how - synchronously?), it is possible that > the file will be removed before we are able to read it. Good points. Fixed. https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:57: Future<List<int>> readFileBytes(); On 2016/10/18 17:36:59, Brian Wilkerson wrote: > > Maybe just readBytes(). > > Unless there's a strong argument to do otherwise I think we should match the > protocol from dart:io so that authors of clients of the front end don't have to > learn a new API. > > > I don't think we need to make a distinction between file existence and reading > > error. Even if we perform a check (how - synchronously?), it is possible that > > the file will be removed before we are able to read it. > > I agree. Any failure to provide the content should throw an exception. I believe > that's the way the dart:io API works, and I think we should be consistent. Good point. You are correct that this is how dart:io works. I've modified the comment to be consistent with this. https://codereview.chromium.org/2426773004/diff/1/pkg/front_end/lib/file_syst... pkg/front_end/lib/file_system.dart:65: Future<String> readFileString(); On 2016/10/18 17:10:32, scheglov wrote: > Again, maybe just readString(). > > We could specify that the file is read as UTF8. > > The same comment about checking that file exists. Done.
LGTM https://codereview.chromium.org/2426773004/diff/20001/pkg/front_end/lib/file_... File pkg/front_end/lib/file_system.dart (right): https://codereview.chromium.org/2426773004/diff/20001/pkg/front_end/lib/file_... pkg/front_end/lib/file_system.dart:46: /// Returns the absolute path represented by this file system entity. A small tweak. /User/foo/../bar/baz.dart is absolute, but not normalized. Maybe we should say "Returns the absolute normalized path" to be exact. https://codereview.chromium.org/2426773004/diff/20001/pkg/front_end/lib/file_... pkg/front_end/lib/file_system.dart:50: /// that was used to create the object, since the path my have been "my have", "might have"? https://codereview.chromium.org/2426773004/diff/20001/pkg/front_end/lib/file_... pkg/front_end/lib/file_system.dart:54: /// Returns a `file:` URI representing this file system entity. It still does not answer my question, whether this URI is the "file:" URI with the [path], or the path it might be different. Not that this is important, I'm just curious to understand. https://codereview.chromium.org/2426773004/diff/20001/pkg/front_end/lib/file_... pkg/front_end/lib/file_system.dart:58: /// that was used to create the object, since the Uri my have been normalized. Same here my/might.
lgtm https://codereview.chromium.org/2426773004/diff/20001/pkg/front_end/lib/file_... File pkg/front_end/lib/file_system.dart (right): https://codereview.chromium.org/2426773004/diff/20001/pkg/front_end/lib/file_... pkg/front_end/lib/file_system.dart:67: Future<List<int>> readBytes(); I still think this should be consistent with dart:io (readAsBytes), but...
https://codereview.chromium.org/2426773004/diff/20001/pkg/front_end/lib/file_... File pkg/front_end/lib/file_system.dart (right): https://codereview.chromium.org/2426773004/diff/20001/pkg/front_end/lib/file_... pkg/front_end/lib/file_system.dart:46: /// Returns the absolute path represented by this file system entity. On 2016/10/18 20:43:40, scheglov wrote: > A small tweak. > /User/foo/../bar/baz.dart is absolute, but not normalized. > Maybe we should say "Returns the absolute normalized path" to be exact. Done. I also modified the documentation on `entityForPath` and `entityForUri` to clarify that paths are absolutized. https://codereview.chromium.org/2426773004/diff/20001/pkg/front_end/lib/file_... pkg/front_end/lib/file_system.dart:50: /// that was used to create the object, since the path my have been On 2016/10/18 20:43:40, scheglov wrote: > "my have", "might have"? Done. https://codereview.chromium.org/2426773004/diff/20001/pkg/front_end/lib/file_... pkg/front_end/lib/file_system.dart:54: /// Returns a `file:` URI representing this file system entity. On 2016/10/18 20:43:40, scheglov wrote: > It still does not answer my question, whether this URI is the "file:" URI with > the [path], or the path it might be different. Not that this is important, I'm > just curious to understand. After some discussion, we decided to remove this getter. If it later turns out to be needed, we can address this issue. https://codereview.chromium.org/2426773004/diff/20001/pkg/front_end/lib/file_... pkg/front_end/lib/file_system.dart:58: /// that was used to create the object, since the Uri my have been normalized. On 2016/10/18 20:43:40, scheglov wrote: > Same here my/might. Acknowledged. https://codereview.chromium.org/2426773004/diff/20001/pkg/front_end/lib/file_... pkg/front_end/lib/file_system.dart:67: Future<List<int>> readBytes(); On 2016/10/18 20:52:03, Brian Wilkerson wrote: > I still think this should be consistent with dart:io (readAsBytes), but... Done.
Description was changed from ========== First cut at a file system abstraction for the front end. ========== to ========== First cut at a file system abstraction for the front end. R=brianwilkerson@google.com, scheglov@google.com Committed: https://github.com/dart-lang/sdk/commit/8fca9ba9683047d7f96f4578a920f8f7c4c30640 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 8fca9ba9683047d7f96f4578a920f8f7c4c30640 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
