Skip to content
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

First attempt at making IChangeContext interface #26

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Jan 29, 2025

Attempting to fix #23. Two problems so far:

  • IChangeContext had to change the type of the Commit property from Commit to CommitBase since the latter lives in SIL.Harmony.Core. But now ChangeContext's Commit property, of type Commit, "does not have the matching return type of CommitBase". How can I make this work?
  • When ObjectSnapshot is moved into SIL.Harmony.Core, its Commit property has to change type from Commit to CommitBase. But now SimpleSnapshot, which references snapshot.Commit.Hash, has an error because the CommitBase type has no Hash property.

@@ -15,7 +16,7 @@
Commit = commit;
}

public Commit Commit { get; }
public Commit Commit { get; } // PROBLEM: Interface is CommitBase. How do I do this?
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can just change the property type from Commit to CommitBase


namespace SIL.Harmony.Core;

public class ObjectSnapshot
Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks like actually moving this would be more painful than I expected. There's also the fact that since it's stored in the Db changing the Commit type might cause problems there. Instead lets just extract an interface. It should have all the properties but the Commit type will be CommitBase on the interface. We then need to make an explicit interface implementation on ObjectSnapshot. That looks like this:
CommitBase IObjectSnapshot.Commit => Commit, this basically makes this Commit property only get used if the object is referred to as IObjectSnapshot if it's referred to as ObjectSnapshot then it will use the public property.

@@ -15,7 +16,7 @@
Commit = commit;
}

public Commit Commit { get; }
public Commit Commit { get; } // PROBLEM: Interface is CommitBase. How do I do this?
public async ValueTask<ObjectSnapshot?> GetSnapshot(Guid entityId) => await _worker.GetSnapshot(entityId);
public async ValueTask<T?> GetCurrent<T>(Guid entityId) where T : class
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

[Re: line +29]

this doesn't compile, it'll need to change to an explicit implementation IObjectBase IChangeContext.Adapt(object obj)

See this comment inline on Graphite.

Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

it looks like a good start. Hopefully my suggestions get you unstuck.

Don't forget about IChange.ApplyChange and IChange.NewEntity they should be changed to use IChangeContext as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make an IChangeContext interface and depend on it from
2 participants