Refactoring to remove late variables and cyclic dependencies

[eliasyishak]

Related to:

• Refactor to remove late variables + clean up cyclic dependencies #241

Changes made in this PR:

• Removed all late variables from AnalyticsImpl, ConfigHandler
• All Initializer methods for creating files have been converted to static methods
• LogHandler and UserProperty includes instance member errorSet to keep track of any errors encountered
  • Using this errorSet instance variable, we are able to delete error_handler.dart and handle the logic within AnalyticsImpl
• Deleted session.dart and consolidated session logic into UserProperty

---

• I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

• See our contributor guide for general expectations for PRs.
• Larger or significant changes should be discussed in an issue before creating a PR.
• Contributions to our repos should follow the Dart style guide and use dart format.
• Most changes should add an entry to the changelog and may need to rev the pubspec package version.
• Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

[eliasyishak]

Referring to:

• https://github.com/google/file.dart/issues/236

[christopherfujino]

Are you doing this solely for test code? Let's try to avoid doing extra file system operations just for the sake of tests.

[eliasyishak]

Yes, a test consistently fails if we don't set the modified timestamp because it thinks there is more than one session since the last modified timestamp gets set to something like 2000-01-01 00:08:00.000

https://github.com/eliasyishak/tools/blob/remove-late-variables/pkgs/unified_analytics/test/unified_analytics_test.dart#L1089

I don't think deleting a valid test because of a bug would is ideal in this case either.

[eliasyishak]

Actually this is not a bug, the dartdocs explicitly say that a fake clock is used for the MemoryFileSystem.test constructor. Currently trying to refactor around this

[eliasyishak]

I've moved the call to File.setLastModifiedSync into the test... where it should have probably been the whole time. Good catch :)

[christopherfujino]

I see this file path being constructed twice, here on line 95 and later on line 174, can we make it a shared static function on the SurveyHandler class that takes in the home path as an argument, that way if/when we change the path we only have to change one place? Or, if the path will always be the same relative to home, can we just pass in the home path to the constructor and have the constructor figure it out?

[eliasyishak]

I actually had this thought as well, except i wanted to do this for all of the file paths we use for the 5 files we generate.

I was thinking of making an Artifacts class with getters for each file, simple example below

import 'package:file/file.dart';
import 'package:file/memory.dart';
import 'package:path/path.dart' as p;

class Artifacts {
  static File dismissedSurveyFile({
    required Directory homeDirectory,
    required FileSystem fs,
  }) =>
      fs.file(p.join(homeDirectory.path, 'path', 'to', 'dismissedSurvey.json'));

  static File logFile({
    required Directory homeDirectory,
    required FileSystem fs,
  }) =>
      fs.file(p.join(homeDirectory.path, 'path', 'to', 'analytics.log'));
}

void main() {
  final fs = MemoryFileSystem.test();
  final homeDirectory = fs.directory('home');

  homeDirectory.createSync();

  final dismissedSurveyFile =
      Artifacts.dismissedSurveyFile(homeDirectory: homeDirectory, fs: fs);
  dismissedSurveyFile.createSync(recursive: true);


  final logFile = Artifacts.logFile(homeDirectory: homeDirectory, fs: fs);
  logFile.createSync(recursive: true);
}

Thoughts?

[eliasyishak]

Or we could create an instance of Artifacts and pass the filesystem and homedirectory in the constructor so we could simply run something like the below

import 'package:file/file.dart';
import 'package:file/memory.dart';
import 'package:path/path.dart' as p;

class Artifacts {
  final Directory homeDirectory;
  final FileSystem fs;

  Artifacts({required this.homeDirectory, required this.fs});

  File get dismissedSurveyFile =>
      fs.file(p.join(homeDirectory.path, 'path', 'to', 'dismissedSurvey.json'));

  File get logFile =>
      fs.file(p.join(homeDirectory.path, 'path', 'to', 'analytics.log'));
}

void main() {
  final fs = MemoryFileSystem.test();
  final homeDirectory = fs.directory('home');

  homeDirectory.createSync();

  // Create an instance that we can pass around
  final _artifacts = Artifacts(homeDirectory: homeDirectory, fs: fs);

  final dismissedSurveyFile = _artifacts.dismissedSurveyFile;
  dismissedSurveyFile.createSync(recursive: true);

  final logFile = _artifacts.logFile;
  logFile.createSync(recursive: true);
}

But either way, I was thinking of adding this in a separate PR

[christopherfujino]

I think overall, I would try to avoid adding new classes unless you have a set of behaviors (functions) that:

1. mutate state that they exclusively own; and/or
2. you need to model a hierarchy of types (e.g. Animal, Dog, Cat)

Thus, I would prefer these just be top level functions

[christopherfujino]

If you decide to keep the dependency on package:file (which I think is reasonable), these functions could simply take one argument, Directory homeDirectory, and then you could specify the path to the desired files via the FileSystemEntity.childDirectory and FileSystemEntity.childFile methods, which will ensure the appropriate path separator is used for the file system your home directory was created from.

[christopherfujino]

Doing this in a separate PR SGTM.

[christopherfujino]

LGTM

[devoncarew]

FYI that it looks like we'll need updates to the dart sdk repo (and google3?) in order to rev to the commit w/ this PR.