Skip to content
This repository has been archived by the owner on Jan 18, 2022. It is now read-only.

Subscription cleanup #1200

Merged
merged 14 commits into from
Oct 25, 2019
Merged

Subscription cleanup #1200

merged 14 commits into from
Oct 25, 2019

Conversation

zeroZshadow
Copy link
Contributor

@zeroZshadow zeroZshadow commented Oct 22, 2019

Description

In trying to debug subscription issues, and split up the package, we've found that its currently in a bit of a state.
This PR hopes to address some of this, by ensuring the code follows our overall style guide and has a clearer API

This is WIP as I'd still like to make some changes to our Callbacks code.

Please review in commit order, I've tried to keep things manageable.

Documentation

  • Changelog

@improbable-prow-robot improbable-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. jira/no-ticket Indicates a PR has no corresponding JIRA ticket size/XL Denotes a PR that changes 300-599 lines, ignoring generated files. labels Oct 22, 2019
@improbable-prow-robot improbable-prow-robot added the A: core Area: Core GDK label Oct 22, 2019

namespace Improbable.Gdk.Subscriptions
{
public static class EntitySubscriptions
Copy link
Contributor

Choose a reason for hiding this comment

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

what was the initial purpose of this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has never been used.

@improbable-prow-robot improbable-prow-robot added size/XXL Denotes a PR that changes 600+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 300-599 lines, ignoring generated files. labels Oct 23, 2019
@zeroZshadow zeroZshadow marked this pull request as ready for review October 25, 2019 09:37
@improbable-prow-robot improbable-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2019
@@ -197,7 +197,7 @@ public void FlushCommandBuffer()
if (!workerSystem.TryGetEntity(entityId, out var entity))
{
throw new ArgumentException(
$"Can not add GameObjet components to entity {entityId}. Entity not in view");
$"Can not add GameObject components to entity {entityId}. Entity not in view");
Copy link
Contributor

Choose a reason for hiding this comment

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

😅

Copy link
Contributor

@jamiebrynes7 jamiebrynes7 left a comment

Choose a reason for hiding this comment

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

Mostly nits - otherwise LGTM

@zeroZshadow zeroZshadow merged commit 82c49b5 into develop Oct 25, 2019
@zeroZshadow zeroZshadow deleted the feature/subscription-fixes branch October 25, 2019 14:49
jessicafalk pushed a commit that referenced this pull request Nov 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A: core Area: Core GDK jira/no-ticket Indicates a PR has no corresponding JIRA ticket size/XXL Denotes a PR that changes 600+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants