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

Refactor to use BasicTask and Operations #428

Merged

Conversation

djmitche
Copy link
Collaborator

This refactors a bunch of Replica methods and the high-level Task/TaskMut to use BasicTask and commit operations to achieve their goals.

I summarize all of the deprecations together in a later commit, with the breaking changes.

This is one of the "boring" bits for #372 - the next is, too, and then we get to the interesting breaking changes.

@djmitche djmitche requested a review from ryneeverett July 21, 2024 14:59
Copy link
Collaborator

@ryneeverett ryneeverett left a comment

Choose a reason for hiding this comment

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

  • I'm seeing a few self.task.task in TaskMut and task.task.task in tests which I believe could be reduced by one ".task" due to Deref.
  • I noticed that clippy is broken in CI again: Error: Unable to create clippy annotations! Reason: HttpError: Resource not accessible by integration and there appear to be some relevant warnings. It sure would be nice if the check would fail.

taskchampion/src/replica.rs Show resolved Hide resolved
taskchampion/src/replica.rs Show resolved Hide resolved
taskchampion/src/task/task.rs Outdated Show resolved Hide resolved
taskchampion/src/task/task.rs Outdated Show resolved Hide resolved
djmitche added 4 commits July 23, 2024 02:15
This refactors a bunch of Replica methods and the high-level
Task/TaskMut to use BasicTask and commit operations to achieve their
goals.
@djmitche djmitche force-pushed the issue372-use-basictask-and-ops branch from cb40df5 to 3d28e61 Compare July 23, 2024 06:18
@djmitche
Copy link
Collaborator Author

I'm seeing a few self.task.task in TaskMut and task.task.task in tests which I believe could be reduced by one ".task" due to Deref.

This doesn't work because deref only works for method calls, so self.task.foo() uses the task property on TaskMut, of type Task, and then looks for a foo method on it. Later PRs will address this, in particular when Task and TaskMut are unified into a single type.

I noticed that clippy is broken in CI again

Oops, thanks for spotting that! A downside of a lot of rebasing.

I have a little more work to do here and will request review.

@djmitche djmitche requested a review from ryneeverett July 23, 2024 06:45
Copy link
Collaborator

@ryneeverett ryneeverett left a comment

Choose a reason for hiding this comment

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

Looks good. I don't entirely understand all the testing changes in the last commit but I presume they pertain to removing apply in the next PR.

@djmitche
Copy link
Collaborator Author

Looks good. I don't entirely understand all the testing changes in the last commit but I presume they pertain to removing apply in the next PR.

Yeah, a lot of the tests are in terms of SyncOp instead of Operation, and that gets addressed in the next PR ("Prefer Operation instead of SyncOp"). So, for this PR I've kept the SyncOp-related methods as test-only.

@djmitche djmitche merged commit e1dc08e into GothenburgBitFactory:main Jul 24, 2024
11 checks passed
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.

2 participants