-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Create a public abstraction for managing seed data #15288
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the whole PR to see if I could come up with ideas, but I am not sure I succeeded.
I feel a bit uneasy with the fact that we need to add a (thin) wrapper to do this, instead of being able to make parts of our existing internal interfaces public, but if there are good reasons, so be it.
@@ -36,7 +36,6 @@ public class CommandBatchPreparer : ICommandBatchPreparer | |||
private readonly IComparer<ModificationCommand> _modificationCommandComparer; | |||
private readonly IKeyValueIndexFactorySource _keyValueIndexFactorySource; | |||
private readonly int _minBatchSize; | |||
private IStateManager _stateManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, was this class more general and now only applies to model data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that the answer is no, and this is what @AndriySvyryd means when he says the new abstractions are not only about data seeding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that is correct.
{ | ||
continue; | ||
} | ||
|
||
ChangeDetector.DetectChanges(stateManager); | ||
var entries = stateManager.GetEntriesToSave(); | ||
modelDataTracker.DetectChanges(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, mostly because I don't remember how this works: Why is it that in this scenario we need to explicitly detect changes but in other usages of the interface we don't need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other places we set the state explicitly and the users doesn't have a chance to mutate the entities therefore DetectChanges
would be noop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but in this one they have a chance? How?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we don't know the end state beforehand. We've just added/removed all entities from the data and rely on DetectChanges
to sort out which ones are updates or noops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. That helped refresh my memory. Anyway, the reason I asked originally is that I was wondering if things would feel cleaner if we kept IChangeDetector as its own separate abstraction (which I guess could be made public). I believe then IModelDataTracker would become a strict subset of IStateManager (i.e. less mix up of responsibilities, and perhaps it would easier to come up with a good name for the public interface). But on the other hand, would it be ok if the input of IChangeDetector.DetectChanges() was an IModelDataTracker?
0233855
to
35fb1a9
Compare
{ | ||
var parameterNameGenerator = _parameterNameGeneratorFactory.Create(); | ||
var commands = CreateModificationCommands(entries, parameterNameGenerator.GenerateNext); | ||
var commands = CreateModificationCommands(entries, modelData, parameterNameGenerator.GenerateNext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't matter a lot, but I think the parameter name modelData
should be updateAdapter
or similar.
Part of #15096 and #15126
To avoid using the state manager directly.