Add samples for all AppConfig pager methods (GetSnapshots, GetConfigurationSettings, GetRevisions) with comparisons of what we'd expect the user experience to be. #6341
Conversation
…rationSettings, GetRevisions) with comparisons of what we'd expect the user experience to be.
ahsonkhan
requested review from
LarryOsterman,
antkmsft,
gearama and
RickWinter
as code owners
|
API change check API changes are not detected in this pull request. |
| keyValuesPage.HasPage(); | ||
| keyValuesPage.MoveToNextPage()) | ||
| { | ||
| if (keyValuesPage.Items.HasValue()) |
There was a problem hiding this comment.
It is beyond AppConfig, and this is not question only to you, but maybe we should have a design discussion with the architects, but I am wondering if we should make pageable items as non-nullable, at least, by default - I don't nullable here is the best user experience, and I don't expect nullable vs size == 0 to ever have meaningful distinction in the context of Azure SDK. Ideally, the users would just go with for (item : pageable.Items) right away, without checking for HasValue().
The spec says it is optional, and I get that, but if we don't take that literally, we could special-case that.
To be extra careful, we could make an option for that, which you could toggle to Off for the potential services where there is a meaningful distinction.
There was a problem hiding this comment.
That's an interesting question. I think I agree with you that Items should just be a vector<T> and not a Nullable<vector<T>>.
My one concern is that I suspect it is legal for a pageable to return 0 entries for a page even if there are more pages still to come (in other words, HasPage is true, but Items is empty. It is a coding mistake for a customer to treat Items.empty() as if it was the same as HasPage() returning false, but it would be an easy mistake for a developer to make. Making Items be nullable basically renders that mistake impossible at the cost of a poor developer experience.
There was a problem hiding this comment.
Hmm, but technically nothing prevents pageable.Items.HasValue() == false while pageable.HasPage() == true. And I also thought another possible mistake for the user to make - to assume that Items.Value().size() > 0 if Items.HasValue() == true. So, it is complicated, for sure, but maybe we could make it a tiny bit better.
Whether the service returns [] or null/undefined, we can generate code to hide that from user, in all cases the result will be as if it was [].
There was a problem hiding this comment.
This is a good conversation. To Larry's point, having an empty page (where HasPage = true) is something we'd need to consider keeping the distinction clear on.
I am reminded that the customer can make the same call directly to the service and would have to handle the null or empty case, even outside of the SDK.
The simplest (and easiest to explain) stance is to honor the spec and service behavior as the source of truth. We could try to introduce usability improvements like this as a potential UX win for SDK users, but I am not sure if we potentially lead to further confusion or unintended consequences.
There was a problem hiding this comment.
To your point, @antkmsft, the existing hand-written SDKs don't use Nullable<T> for the collection: https://github.com/Azure/autorest.cpp/issues/475
Part of #6295, #6299, #6291, #6302
The expected user experience is under
#if 0for now since it won't compile.The goal here is to leverage the hero scenario samples for AppConfig to concretely highlight and track the gap between what is generated today vs what we want the end user experience and API design to be.
No new emitter or API design issues discovered.
One service issue with missing documented functionality in the spec:
#6340