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

Remove unused code from cadence.internal package #921

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

natemort
Copy link
Member

What changed?
Remove unused code from internal packages to improve code coverage and reduce maintenance burden. Users shouldn't be accessing internals.

Why?
Improving code coverage and reducing maintenance burden

How did you test it?
Unit tests

Potential risks
User code may be accessing internals within the client and could break. Generally this should be a compile time breakage and we do not support accessing internals.

Release notes

Documentation Changes

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (master@6d3f55f). Learn more about missing BASE report.

Additional details and impacted files
Files with missing lines Coverage Δ
...adence/internal/common/WorkflowExecutionUtils.java 46.55% <ø> (ø)
.../internal/replay/ActivityDecisionStateMachine.java 77.41% <ø> (ø)
...rnal/replay/ChildWorkflowDecisionStateMachine.java 45.61% <ø> (ø)
...ce/internal/replay/SignalDecisionStateMachine.java 57.14% <ø> (ø)
...nce/internal/replay/TimerDecisionStateMachine.java 80.00% <ø> (ø)
...ternal/sync/SimulatedTimeoutExceptionInternal.java 100.00% <ø> (ø)
...uber/cadence/internal/sync/SyncWorkflowWorker.java 81.81% <ø> (ø)
...m/uber/cadence/internal/sync/WorkflowInternal.java 87.62% <ø> (ø)
...cadence/internal/sync/WorkflowRetryerInternal.java 83.05% <ø> (ø)
...va/com/uber/cadence/internal/worker/Throttler.java 0.00% <ø> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d3f55f...c937660. Read the comment docs.

@coveralls
Copy link

coveralls commented Oct 17, 2024

Pull Request Test Coverage Report for Build 2542

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 8 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.4%) to 65.75%

Files with Coverage Reduction New Missed Lines %
src/main/java/com/uber/cadence/internal/sync/WorkflowRetryerInternal.java 1 86.44%
src/main/java/com/uber/cadence/internal/sync/WorkflowThreadContext.java 1 81.58%
src/main/java/com/uber/cadence/internal/worker/Poller.java 1 78.13%
src/main/java/com/uber/cadence/internal/replay/ReplayDecider.java 5 80.27%
Totals Coverage Status
Change from base Build 2537: 0.4%
Covered Lines: 12741
Relevant Lines: 19378

💛 - Coveralls

Copy link

@3vilhamster 3vilhamster 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 unsure if we want to delete this, but I could not find usages of this in monorepos or in open sourced usages.

@@ -621,26 +437,6 @@ public static GetWorkflowExecutionHistoryResponse getHistoryPage(
return history;
}

/** Returns workflow instance history in a human readable format. */
public static String prettyPrintHistory(

Choose a reason for hiding this comment

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

This could be useful; I wonder why have you decided to drop this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's potentially useful but it's long unused and untested. If we need similar functionality we can rewrite it.

* @param <R> activity return type
* @return activity result
*/
public static <R> R executeActivity(

Choose a reason for hiding this comment

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

I wonder why is this removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not really sure why this exists, it's long unused and untested. I think it's best we just remove it.

@natemort natemort force-pushed the deadcode branch 2 times, most recently from d895c5f to 3fdf3d6 Compare October 24, 2024 15:11
@natemort natemort merged commit 8693c37 into cadence-workflow:master Oct 24, 2024
13 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.

4 participants