Introduce Services #395
Merged
Introduce Services #395
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Since there are still some things that need to be worked out with |
When the `Runner` and `ManifestService` changed to use `yield return`, it seems to have accidentally resulted in producing outputs for multiple manifest files from multiple providers. That something that we ultimately want, and I'm trying to keep this pull request more tightly scoped than that.
When I moved the LibYearCalculator creation logic out of the ManifestFinder (now ManifestService) class, I incorrectly passed the single manifest file in to the `RepositoryFor` and `ManifestFor` methods, when those methods are expecting to work on directories.
mscottford
force-pushed
the
introduce-services
branch
from
39283d3 to
8ee6858
Compare
This will be changed by the date-time-offset branch when it's ready, so it's okay to accept this change here.
This will be fixed when multiple-manifest support is added, so we can defer this for now.
Introduces `FileHistoryFinderRegistry` that's responsible for keeping track of the diffrent `IFileHistoryFinder` instances that have been registered. Renames `FileHistoryFinder` class to `FileHistoryService` to better match the role that it plays. Also moves time-intensive tasks out of the class's construct. This is to support the shift to `asnyc` calls that will come later. Constructors cannot run in an `async` fashion, so anything that might to be `await`ed on needs to be moved out of the constructor.
Before the registy class was introduced, there was an assumption that the `LocalFileHistoryFinder` was always registered. This default behavior has been removed. Also took an incremental step towards more dependency injection. This is causing more setup logic to appear in the tests. That logic can be centralized at a later time.
The `LocalFileHistoryFinder` needs to be registered last. This is because it always reports that it can work with the specified path. This makes it closer to an implementation of the Null Object pattern.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Converts usage of
ManifestFinderandFileHistoryFinderinto "services" that can be used to obtain finder instances. Also introducesManifestFinderRegistryandFileHistoryFinderRegistryto separate that responsibility from the service. Also changes the way that the service is interacted with, to make it less aware of some implementation details. Also starts laying the groundwork for using Dependency Injection more broadly.Dependent on #298 and is branched off of its branch. Will be rebased against
mainwhen that PR is merged.