Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Docs: Lifecycle Management of Counters and Lists in REST #3560
Docs: Lifecycle Management of Counters and Lists in REST #3560
Changes from 1 commit
ba37f1c
8875016
f51a108
c346e85
ee7ee78
aee6dc3
fcc71be
561f1e4
0a60b0c
8bdb9d9
f164d4f
b83ad2f
f7c29a3
75e5569
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@igooch @Kalaiselvi84 how do you feel about this suggestion? (and then repeating it through all the below REST calls as well) -- then it's doubly reinforced what the key is, both above and inline in the description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made the suggested changes. Ivy, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably mention in the example that conformanceTestCounter is predefined in the local SDK Server, or go with the more generic example of {name} or {counterName}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have included the Local Development Setup section for Counters and Lists in this document. Could you please review it and share your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add an explainer under each example that uses a key, something like:
Since people don't always read the top level docs 😃
As an improvement to the docs and likely the local sdk experience, @igooch I didn't catch this earlier, but any objection to changing:
agones/pkg/sdkserver/localsdk.go
Lines 147 to 158 in 1e78d88
To using some more generic keys? Maybe
players
for the list androoms
for the count?We don't actually use these values for conformance tests anyway, so this might be a bit confusing, and I think would make examples easier to understand. WDYT? If so, we can do this in a separate PR (and will need to feature shortcode the new docs) - if you like we can file a new issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have included this information in a
Note
section located under both theCounters
andLists
titles, following theWarning
section. This will avoid the repetition right? WDYT?Do you want to replace
"conformanceTestCounter": {Count: 1, Capacity: 10}}
with"rooms": {Count: 1, Capacity: 10}}
and"conformanceTestList": {Values: []string{"test0", "test1", "test2"}, Capacity: 100}}
with"players": {Values: []string{"player1", "player2", "player3"}, Capacity: 100}}
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm silly - of course we use this for conformance tests 🤦🏻 . Ignore that comment. But I am wondering if we change the key anyway, to make it easier for end users, and make our conformance testing match. WDYT?
For these docs, we don't have to conform to the local SDK for the example, so I think we can change the key to something more generic like I suggest above. I'd like to get agreement on the code changes from @igooch before we make the code changes though to the local sdk server (and either way, we should handle that in a separate PR).
Sound good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My personal preference is to have the example match what we have in the localSDKServer for a better user experience. That way the user can run the example code and it works without them needing to make changes. My general gripe with generic examples is that they implicitly assume the user knows what and where to make changes without any accompanying instructions. That being said the key "conformanceTestCounter" probably doesn't mean anything to the user either, so I'd rename the keys along the lines of "TestCounterKey", "TestPlayerCounter", etc.
I'm also fine leaving the examples more generic here, as long as we have a step by step guide elsewhere that is more accessible to the user that needs more context on how and where to make changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% agreed - no disagreement there - sounds like we're just talking about what names to give keys. This change I've suggested above I would like to see as an interim step until we can align it again next release with a future PR to update the local sdk server and conformance tests.
So i would argue that both "test", "key", and "counter" are redundant in the name, and just confuse things a bit more, and it's easier to give a real world use case for the local SDK user that is easy to understand (and will align with the eventual integration pattern docs I'll write - primarily one for "rooms" and one for "player tracking" - as well as the examples we have in the yaml files etc.).
That being said, I would be super happy if as part of the description for each method, there is a reminder for what the key is that is being used, since reminders are great and redundancy in docs is definitely not a bad thing, I'll write up a small suggestion, lemme know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sticking with
rooms
andplayers
for now and considering changes later if needed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just another example of the change style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be a good idea if we rephrase like this?
This function updates the properties of the counter with the key
rooms
, such as its count and capacity, and returns the updated counter details.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That reads better! Nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.