Skip to content

Conversation

anne-edwards
Copy link
Contributor

@anne-edwards anne-edwards commented Dec 20, 2018

Couple of questions/suggestions:

  • Is there any reason why sometimes the summary list is at the top, and sometimes it's lower down, below all the "PLEASE REPLACE"s? Being at the top makes sense to me, but I wanted to check.

  • What do you think about potentially changing the wording in the GitHub steps so that it matches the wording in the docs? In some cases, the GitHub steps and the docs steps don't actually match each other, whereas in other cases they match up but use inconsistent wording.
    E.g. at the moment the "Player authentication" scenario is like this in the docs:

  1. Instantiate clients
  2. Create a player identity token (PIT) for a project
  3. Choose a deployment that the player is allowed to join
  4. Create a login token (LT) to grant access to a player to a given SpatialOS deployment
  5. Connect to the deployment using both PIT and LT

but like this in the code:

  1. Start a cloud deployment.
  2. Create a PlayerIdentityToken.
  3. Choose a deployment that is ready for login.
  4. Create a LoginToken for a selected deployment.
  5. Connect to the deployment using the PlayerIdentityToken and the LoginToken.

@anne-edwards anne-edwards changed the title Updating scenario wording TC-137 Update platform SDK scenario wording Dec 20, 2018
@anne-edwards anne-edwards requested a review from lx223 December 20, 2018 14:18
@lx223
Copy link
Contributor

lx223 commented Dec 20, 2018

@anne-edwards

The default C# formatter orders those fields differently, depending on their modifiers.

For example, in private const string ProjectName, ProjectName is modified to be private const string whereas in private static string RefreshToken, RefreshToken, private static string. The default formatter would group them together and re-order them. But, I agree with you that it would be nicer to have all the REPLACE at the top.

The scenario doc and code don't always align. The code has to be fully working so it tends to have a few more set up like Starting a cloud deployment which we wouldn't have to put in the doc so as not to clutter a walk-through scenario. But, wherever they do align, I think it would make sense for them to share the same wording.

Copy link
Contributor

@lx223 lx223 left a comment

Choose a reason for hiding this comment

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

Change LGTM.

/// PLEASE REPLACE.
/// Your SpatialOS project name.
/// It should be the same as the name specified in the local spatialos.json file used to start the local API services.
/// It should be the same as the name specified in the local project definition file (spatialos.json) used to start the local API service ("spatiald").
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you currently have it documented somewhere the name of the process? If not, mentioning spatiald probably just adds confusion without providing much value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't define it anywhere; I added it here so that users would at least be familiar with the term when they see it in the code.

However, having just searched in the code, it only appears a few times (as SpatialdEndpoint and SpatialDPort) so maybe it's not worth drawing attention to?

Or, I could define both "spatiald" and "local API service" in the main SpatialOS glossary and explain that they're the same thing, but that "SpatialD" and "Spatiald" are used in the code?

@anne-edwards
Copy link
Contributor Author

@lx223 thanks for the explanations.

Regarding the ordering, is there any (potentially hacky) way we could make it so that all the summaries are at the top? No worries if not.

Where the scenario and doc align, I'll update one of them for consistency.

@bradleyjkemp bradleyjkemp merged commit 4ddc6f3 into master Jan 11, 2019
@bradleyjkemp bradleyjkemp deleted the docs/update-scenario-wording branch January 11, 2019 10:36
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.

3 participants