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.

Sign up for GitHub

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

Jump to bottom

Remove context.subscribe #1111

Merged
merged 1 commit into from
Apr 27, 2020
Merged

Remove context.subscribe #1111

merged 1 commit into from
Apr 27, 2020

Conversation

AquaGeek
Copy link
Contributor

This is a rebase of #1025. It removes context.subscribe altogether, now that we've migrated our internal clients. This is a breaking change.

This finishes the work on #402 / #744.

@AquaGeek AquaGeek added the swift Affects the Swift library. label Apr 27, 2020
JustinDSN
JustinDSN reviewed Apr 27, 2020
View reviewed changes
swift/Workflow/Sources/SubtreeManager.swift
// MARK: - Subscriptions

extension WorkflowNode.SubtreeManager {
fileprivate final class Subscriptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

Bye bye bye 👋

Copy link
Contributor

Choose a reason for hiding this comment

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

The red lines make me happy :D

JustinDSN
JustinDSN approved these changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@JustinDSN JustinDSN 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 to me!

@JustinDSN
Copy link
Contributor

JustinDSN commented Apr 27, 2020 •
edited
Loading

The 8 commits are a little weird. Do we need all of them? Should we squash them into a single commit?

dhavalshreyas
dhavalshreyas approved these changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@dhavalshreyas dhavalshreyas left a comment

Choose a reason for hiding this comment

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

👋

swift/Workflow/Sources/SubtreeManager.swift
// MARK: - Subscriptions

extension WorkflowNode.SubtreeManager {
fileprivate final class Subscriptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

The red lines make me happy :D

@AquaGeek
Copy link
Contributor Author

The 8 commits are a little weird. Do we need all of them? Should we squash them into a single commit?

We don't need all of them. Some got pulled out into the other PR that introduced SignalWorker. I tried rebasing and squashing some of them, but there were conflicts. I'll just squash all of them.

@AquaGeek AquaGeek force-pushed the stromberg/remove-context-subscribe branch from a02a2ec to 41d6a46 Compare April 27, 2020 21:45
@AquaGeek
Remove context.subscribe
6701aaa 
This has been replaced with SignalWorker.
@AquaGeek AquaGeek force-pushed the stromberg/remove-context-subscribe branch from 41d6a46 to 6701aaa Compare April 27, 2020 22:06
@AquaGeek AquaGeek merged commit 8e8bed5 into master Apr 27, 2020
@AquaGeek AquaGeek deleted the stromberg/remove-context-subscribe branch April 27, 2020 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhavalshreyas dhavalshreyas dhavalshreyas approved these changes

@JustinDSN JustinDSN JustinDSN approved these changes

@asthatrivedi asthatrivedi Awaiting requested review from asthatrivedi

Assignees
No one assigned
Labels
swift Affects the Swift library.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AquaGeek @JustinDSN @dhavalshreyas