Skip to content
This repository was archived by the owner on Dec 9, 2024. It is now read-only.

Monotonic clock version of MemoryFileSystem #129

Merged
merged 2 commits into from
Oct 11, 2019

Conversation

Hixie
Copy link
Contributor

@Hixie Hixie commented Oct 9, 2019

No description provided.

/// Each time [now] is called, the time increases by one minute.
///
/// The `start` argument can be used to set the seed time for the clock.
/// The first value will be that time plus one minute.
Copy link
Contributor

Choose a reason for hiding this comment

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

Document what the default start value is.

@@ -35,16 +37,44 @@ abstract class MemoryFileSystem implements StyleableFileSystem {
/// The file system will be empty, and the current directory will be the
/// root directory.
///
/// The clock will be a real-time clock, file modification times will
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: s/,/;/

///
/// The `start` argument can be used to set the seed time for the clock.
/// The first value will be that time plus one minute.
factory MemoryFileSystemClock.monotonicTest() = _MonotonicTestMemoryFileSystemClock;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this allow the caller to specify the start 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.

I believe so. It's just a redirecting constructor.

@@ -46,6 +47,10 @@ abstract class NodeBasedFileSystem implements StyleableFileSystem {
/// The path of the current working directory.
String get cwd;

/// The clock to use when finding the current time (e.g. to set the creation
/// time of a new node).
MemoryFileSystemClock get clock;
Copy link
Contributor

Choose a reason for hiding this comment

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

NodeBasedFileSystem is layered beneath MemoryFileSystem, so we shouldn't refer to a "memory file system clock" here.

Since the interface of the clock has nothing to do with memory file system, what about naming it Clock, of if you're worried about naming collisions, maybe SimpleClock, TimeProvider, ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used Clock.

@Hixie
Copy link
Contributor Author

Hixie commented Oct 11, 2019

The dartfmt check is requiring a net worse formatting:

diff --git a/packages/file/lib/src/backends/memory/memory_file_system.dart b/packages/file/lib/src/backends/memory/memory_file_system.dart
index f69674f..38f818e 100644
--- a/packages/file/lib/src/backends/memory/memory_file_system.dart
+++ b/packages/file/lib/src/backends/memory/memory_file_system.dart
@@ -44,10 +44,11 @@ abstract class MemoryFileSystem implements StyleableFileSystem {
   /// style. The default is [FileSystemStyle.posix].
   factory MemoryFileSystem({
     FileSystemStyle style = FileSystemStyle.posix,
-  }) => _MemoryFileSystem(
-    style: style,
-    clock: const Clock.realTime(),
-  );
+  }) =>
+      _MemoryFileSystem(
+        style: style,
+        clock: const Clock.realTime(),
+      );

   /// Creates a new `MemoryFileSystem` that has a fake clock.
   ///
@@ -61,10 +62,11 @@ abstract class MemoryFileSystem implements StyleableFileSystem {
   /// style. The default is [FileSystemStyle.posix].
   factory MemoryFileSystem.test({
     FileSystemStyle style = FileSystemStyle.posix,
-  }) => _MemoryFileSystem(
-    style: style,
-    clock: Clock.monotonicTest(),
-  );
+  }) =>
+      _MemoryFileSystem(
+        style: style,
+        clock: Clock.monotonicTest(),
+      );
 }

 /// Internal implementation of [MemoryFileSystem].
@@ -73,8 +75,8 @@ class _MemoryFileSystem extends FileSystem
   _MemoryFileSystem({
     this.style = FileSystemStyle.posix,
     @required this.clock,
-  }) : assert(style != null),
-       assert(clock != null) {
+  })  : assert(style != null),
+        assert(clock != null) {
     _root = RootNode(this);
     _context = style.contextFor(style.root);
   }

...but I've applied it.

The test failure appears to be unrelated to this PR?

@Hixie
Copy link
Contributor Author

Hixie commented Oct 11, 2019

dartfmt also required this:

diff --git a/packages/file/test/memory_test.dart b/packages/file/test/memory_test.dart
index c81a1cd..dfa76bf 100644
--- a/packages/file/test/memory_test.dart
+++ b/packages/file/test/memory_test.dart
@@ -62,28 +62,38 @@ void main() {
             fs.directory('C:\\foo').toString(), "MemoryDirectory: 'C:\\foo'");
       });

       test('Link', () {
         expect(fs.link('C:\\foo').toString(), "MemoryLink: 'C:\\foo'");
       });
     });
   });

   test('MemoryFileSystem.test', () {
-    final MemoryFileSystem fs = MemoryFileSystem.test(); // creates root directory
+    final MemoryFileSystem fs =
+        MemoryFileSystem.test(); // creates root directory
     fs.file('/test1.txt').createSync(); // creates file
     fs.file('/test2.txt').createSync(); // creates file
     expect(fs.directory('/').statSync().modified, DateTime(2000, 1, 1, 0, 1));
-    expect(fs.file('/test1.txt').statSync().modified, DateTime(2000, 1, 1, 0, 2));
-    expect(fs.file('/test2.txt').statSync().modified, DateTime(2000, 1, 1, 0, 3));
+    expect(
+        fs.file('/test1.txt').statSync().modified, DateTime(2000, 1, 1, 0, 2));
+    expect(
+        fs.file('/test2.txt').statSync().modified, DateTime(2000, 1, 1, 0, 3));
     fs.file('/test1.txt').createSync();
     fs.file('/test2.txt').createSync();
-    expect(fs.file('/test1.txt').statSync().modified, DateTime(2000, 1, 1, 0, 2)); // file already existed
-    expect(fs.file('/test2.txt').statSync().modified, DateTime(2000, 1, 1, 0, 3)); // file already existed
+    expect(fs.file('/test1.txt').statSync().modified,
+        DateTime(2000, 1, 1, 0, 2)); // file already existed
+    expect(fs.file('/test2.txt').statSync().modified,
+        DateTime(2000, 1, 1, 0, 3)); // file already existed
     fs.file('/test1.txt').writeAsStringSync('test'); // touches file
-    expect(fs.file('/test1.txt').statSync().modified, DateTime(2000, 1, 1, 0, 4));
-    expect(fs.file('/test2.txt').statSync().modified, DateTime(2000, 1, 1, 0, 3)); // didn't touch it
-    fs.file('/test1.txt').copySync('/test2.txt'); // creates file, then mutates file (so time changes twice)
-    expect(fs.file('/test1.txt').statSync().modified, DateTime(2000, 1, 1, 0, 4)); // didn't touch it
-    expect(fs.file('/test2.txt').statSync().modified, DateTime(2000, 1, 1, 0, 6));
+    expect(
+        fs.file('/test1.txt').statSync().modified, DateTime(2000, 1, 1, 0, 4));
+    expect(fs.file('/test2.txt').statSync().modified,
+        DateTime(2000, 1, 1, 0, 3)); // didn't touch it
+    fs.file('/test1.txt').copySync(
+        '/test2.txt'); // creates file, then mutates file (so time changes twice)
+    expect(fs.file('/test1.txt').statSync().modified,
+        DateTime(2000, 1, 1, 0, 4)); // didn't touch it
+    expect(
+        fs.file('/test2.txt').statSync().modified, DateTime(2000, 1, 1, 0, 6));
   });
 }

Copy link
Contributor

@tvolkert tvolkert left a comment

Choose a reason for hiding this comment

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

LGTM!

@tvolkert tvolkert merged commit d35d82d into dart-archive:master Oct 11, 2019
@Hixie Hixie deleted the memory-clock branch October 11, 2019 03:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants