-
Notifications
You must be signed in to change notification settings - Fork 813
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
Tweaks to Client SDK reference #3541
Tweaks to Client SDK reference #3541
Conversation
|
||
This function removes the specific string value string from a List's values. | ||
|
||
An error is returned if the string does not exist in the list. |
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.
(not for this release, just food for thought) - depending on feedback, we may want to revisit some of the handling of thing like returning an error if the value is already in the list. I feel like a non-error return of false
(or language equivalent) in this scenario is a better developer experience, since we can't do very good cross platform enumeration of different types of errors? 🤔
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 thinking it would probably make most sense just to move this all over to the specific Go SDK reference, since that's really what these methods are.
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 always have a high level reference that is (generally) the basis for all language SDK guides. This makes building language agnostic integration guides easier, and general concept onboarding also easier, since we don't have to duplicate as much language across language SDK references.
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.
Or are you saying, let's just move the existing test over to the Go SDK? (now I read your comment again @igooch )
My point was more - we might want to tweak the Go SDK itself in terms of how it does error handling vs boolean returns, which may impact how the backing gRPC/REST client operates. But like I said - not for this release, just something to think about. I might file an issue for discussion after playing with this more once it's released.
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 talking about the reference guide modified in this PR, not any test. These methods are specific to the Go client.
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.
These methods are specific to the Go client.
They are also specific to what will be built for the other language SDKs as well. That's why I made them more generic, so they could be used as the conceptual basis for pretty much all language SDKs.
The Go SDK will have it's own generated docs for it's own language specific reference.
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.
For example, we link off to https://pkg.go.dev/agones.dev/agones/sdks/go#Alpha.GetListCapacity (which already got generated in the last release)
Build Failed 😱 Build Id: fd42e6a0-3bae-4822-b156-f317a3f97bd7 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
okay, so that's a blocker. Looking at a fix now. |
Build Succeeded 👏 Build Id: 15edb576-3c13-4506-ba05-165456c10b16 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Went through the SDK reference: * Reduced documentation length by setting top level comments or removing what (I think) are redundant statements to make consumption and comprehension easier. * Some of the docs were Go SDK specific. Made it more generic (mostly around return of booleans), as different languages may handle SDK commands slightly differently. * Put player tracking under counter and lists, since counters and lists is the replacement * Added a note on counters and lists replacing player tracking. * Consistency with capitalisation. Work on googleforgames#2716
89bb821
to
0175ea2
Compare
Build Failed 😱 Build Id: f52224d7-304e-4ef6-be01-b72a8df49693 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
generic-1.28 was flaking a bunch
|
Build Failed 😱 Build Id: 109f00ec-ed32-421e-b9f9-85d13a479bad To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Okay, this seems like a consistent failure... weird.
|
I cleared out the 1.28-generic cluster, let's see if that improves things. |
Build Succeeded 👏 Build Id: 88e81639-1ea3-43a0-8a05-b57a6dd29533 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
LGTM (but not going to merge yet to give @igooch a chance to respond to your comments). |
What type of PR is this?
/kind documentation
What this PR does / Why we need it:
Went through the SDK reference:
Which issue(s) this PR fixes:
Work on #2716
Special notes for your reviewer:
Please let me know if you disagree with any of the changes.