[storage-blob] add missing jsdocs & export types that were missed out #5857

chradek · 2019-10-26T22:54:08Z

Exports the APIs that were missing from the index.ts file, and adds TSDoc comments where needed.

One caveat...I did not export the abstract StorageClient class from index.ts, since I don't think we actually want users to use it directly. If we do want to export it we'll have to export the StorageClientContext (and possibly other interfaces) as well.

ramya-rao-a · 2019-10-27T00:12:54Z

sdk/storage/storage-blob/src/BrowserPolicyFactory.ts

@@ -3,6 +3,7 @@

 import { RequestPolicy, RequestPolicyFactory, RequestPolicyOptions } from "@azure/core-http";
 import { BrowserPolicy } from "./policies/BrowserPolicy";
+export { BrowserPolicy };


Why is this being exported?
From what I can see, the BrowserPolicy is used only in the BrowserPolicyFactory which in turn is internal to the library and not exposed to the user

There may be a mistake then, because some of the policy factories are currently being exported. Are the factories meant to be exported?

I exported this and the RetryPolicy because the factories referenced them and were being exported for users.

That's interesting. @bterlson are we ok with these policies being exported?

If we are exporting it, then we should probably add a "Storage" prefix to it, right?
Especially for the retry policy.

cc @bterlson

BrowserPolicy should be exported since it's a policy specific to Storage and users might want to compose their own pipeline with BrowserPolicy in it.

StorageBrowserPolicy makes sense (and so too does a general guideline requiring some prefix for policies so it's clear their service-specific), but considering that they can only import this from the storage package itself, it's not super important I think.

Renamed to StorageBlobBrowserPolicy since the policies aren't shared across storage packages.

The rename is part of #5862

ramya-rao-a · 2019-10-27T00:13:49Z

sdk/storage/storage-blob/src/RetryPolicyFactory.ts

@@ -5,6 +5,7 @@ import { RequestPolicy, RequestPolicyFactory, RequestPolicyOptions } from "@azur
 import { RetryPolicy, RetryPolicyType } from "./policies/RetryPolicy";

 export { RetryPolicyType } from "./policies/RetryPolicy";
+export { RetryPolicy };


Why is this being exported?
From what I can see, the RetryPolicy is used only in the RetryPolicyFactory which in turn is internal to the library and not exposed to the user

The RetryPolicyFactory/RetryPolicy should be exported as it is useful when composing a custom pipeline.

Renamed to StorageBlobRetryPolicy since storage queue exports its own retry policy as well.

Rename is part of #5862

ramya-rao-a · 2019-10-27T00:14:56Z

sdk/storage/storage-blob/src/generated/src/models/index.ts

@@ -24,7 +23,7 @@ export interface KeyInfo {
 }

 /**
- * A user delegation key
+ * A user delegation key.


Please revert the changes to this file. It is auto-generated code and will get overwritten the next time we generate the same.

We should log a separate issue to track docs for auto-generated entities and fix the swagger/transformations as part of that.

@XiaoningLiu Is there any other way to improve the docs in the generated models?

@joheredi had fixed comments on the generated model by updating the swagger with a transformation in #5742

@HarshaNalluru Is this the suggested way to edit the comments on a generated model?

Ah, I should have realized that this was a generated file. I'll revert the changes to this file.

Keep a copy of it somewhere, such that if we do decide to update the transformations, we have something to refer to :)

Though we can add transformations in readme like #5742, adding too many transformations for the comments doesn't make sense. Ideally, all these comments must be updated in the original swagger JSON file (other languages will also likely get benefitted by this).

Options -

Add transformations in swagger readme like [Storage][Blob] Fix comment on BlobGetPropertiesHeaders.contentLength #5742 and the generated code will have the docs updated.

Update swagger JSON file with these comments ASAP and regenerate the code before GA.

If we don't want to update the swagger JSON at this point of time(keeping GA in mind),
We can consider updating the JS Docs like the way Chris did in this PR, assuming there won't be any significant updates to the swagger just before GA(which means code-gen won't change).
[ and track the future work to update the swagger JSON with these doc-comments updates and to regenerate the code. ]

Not update the docs for code-gen files and keep this work-item for post GA and fix the docs in swagger as a high priority and do a quick minor/patch release right after GA.

I added a commit to this PR to remove the comments in the generated file, but we can remove that commit if we decide to go ahead with the docs and fast-follow on updating the swagger file.

I would not want to add transformations based on the sheer number of updates we'd have to make.

I agree. I'd suggest option-3 (and making a separate PR for it just for the generated files).

cc - @XiaoningLiu

HarshaNalluru · 2019-10-27T11:07:28Z

sdk/storage/storage-blob/src/generatedModels.ts

  AppendPositionAccessConditions,
  ServiceGetUserDelegationKeyHeaders,
  ServiceSubmitBatchHeaders,
+  ServiceGetAccountInfoHeaders,
+  ServiceGetPropertiesHeaders,


jeremymeng · 2019-10-27T15:24:27Z

There are types that are not directly used in the public api, but cause warnings in api etractor review reports. I chose not to export them because I don't think those types are likely to be used by our users. The structure of the response results are more important.

Sent from my Windows 10 device From: Harsha Nalluru<mailto:notifications@github.com> Sent: Sunday, October 27, 2019 4:07 To: Azure/azure-sdk-for-js<mailto:azure-sdk-for-js@noreply.github.com> Cc: Jeremy Meng<mailto:Jeremy.Meng@microsoft.com>; Review requested<mailto:review_requested@noreply.github.com> Subject: Re: [Azure/azure-sdk-for-js] [storage-blob] add jsdocs (#5857) @HarshaNalluru commented on this pull request.

________________________________ In sdk/storage/storage-blob/src/generatedModels.ts<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAzure%2Fazure-sdk-for-js%2Fpull%2F5857%23discussion_r339336871&data=02%7C01%7CJeremy.Meng%40microsoft.com%7Ce8c4964a80a24c1684c408d75acddd4c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637077712539272143&sdata=XoT7fhj8AJzbqCzzAroHeH34ltp1pSGJpyK8SPkd2WM%3D&reserved=0>:

AppendPositionAccessConditions,

ServiceGetUserDelegationKeyHeaders, ServiceSubmitBatchHeaders, + ServiceGetAccountInfoHeaders, + ServiceGetPropertiesHeaders, ? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAzure%2Fazure-sdk-for-js%2Fpull%2F5857%3Femail_source%3Dnotifications%26email_token%3DABZ3QX77L3BCG6UVZ6D4OMLQQVY7HA5CNFSM4JFPFYGKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJKRQFA%23pullrequestreview-307566612&data=02%7C01%7CJeremy.Meng%40microsoft.com%7Ce8c4964a80a24c1684c408d75acddd4c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637077712539282138&sdata=ONmf%2FEs6wZzXqiT1rcHNntmd2O1mAdejU%2BEmdwqqfuI%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABZ3QX4ISUW24CIWKCY74JDQQVY7HANCNFSM4JFPFYGA&data=02%7C01%7CJeremy.Meng%40microsoft.com%7Ce8c4964a80a24c1684c408d75acddd4c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637077712539282138&sdata=Q%2FyE48wSLkd%2B%2FYjJnV85BxaeYtQhJ4dvYK5yOYX9I8A%3D&reserved=0>.

chradek · 2019-10-27T17:17:47Z

There are types that are not directly used in the public api, but cause warnings in api etractor review reports. I chose not to export them because I don't think those types are likely to be used by our users. The structure of the response results are more important.

Why would we not want to export these though? Are we concerned these interface names may change in the future?

I noticed the typings that are used today are not the rolled-up ones. So, type inference does work, but if a user wanted to reference a type that isn't exported from index.ts they end up having to reach into our package:

import {BlobServiceClient, SharedKeyCredential} from '@azure/storage-blob';
import { ServiceGetAccountInfoHeaders } from '@azure/storage-blob/typings/src/generated/src/models';

async function getAccountParsedHeaders(): Promise<ServiceGetAccountInfoHeaders> {
  const client = new BlobServiceClient(`url`, new SharedKeyCredential('account', 'key'));
  const result = await client.getAccountInfo();
  
  // return the parsed headers...
  return result._response.parsedHeaders;
}

My fear with the above is that now the typings will break if we ever change the internal structure of our package.

If we did use the rollup-typings (Should we be? We do in the other SDK packages), these interfaces actually get pulled into the flattened typings so are available from the root of the package. In this case, exporting them from the index.ts file gets rid of the ae-forgotten-export warnings.

bterlson · 2019-10-27T17:34:27Z

I tend to think those types should be exported from the root.

@chradek re: question about rollup types, I think we should do that eventually, but I wasn't going to push for pre-GA it since api-extractor was broken with storage until @jeremymeng fixed it very recently.

This reverts commit 6885a67.

This reverts commit 26218bf.

chradek · 2019-10-27T19:38:15Z

Reverted policy renames in this PR. Will post in separate PR.

HarshaNalluru · 2019-10-27T20:50:13Z

sdk/storage/storage-blob/src/BlobClient.ts

@@ -1356,6 +1362,7 @@ export class BlobClient extends StorageClient {
   * This method returns a long running operation poller that allows you to wait
   * indefinitely until the copy is completed.
   * You can also cancel a copy before it is completed by calling `cancelOperation` on the poller.
+   * Note that attempting to cancel a completed copy will result in an error being thrown.


ramya-rao-a · 2019-10-27T21:19:25Z

sdk/storage/storage-blob/src/generated/src/models/index.ts

-       * The parsed HTTP response headers.
-       */
-      parsedHeaders: ServiceSetPropertiesHeaders;
-    };


formatter removes the whitespace? :(

Unfortunately :( But plus side, we can refer back to this PR for these docs when we add them to the swagger file!

jeremymeng · 2019-10-27T21:39:09Z

Are we concerned these interface names may change in the future?

Yeah I was thinking that we might need to rename these codegen types so was trying to reduced the exposed surface.

XiaoningLiu

[storage-blob] add jsdocs

d9705ea

chradek requested review from HarshaNalluru, jeremymeng, jiacfan, vinjiang and XiaoningLiu as code owners October 26, 2019 22:54

chradek requested a review from ramya-rao-a October 26, 2019 22:54

configure spelling mistake corrected

566b8b3

ramya-rao-a reviewed Oct 27, 2019

View reviewed changes

HarshaNalluru reviewed Oct 27, 2019

View reviewed changes

revert changes to generated file

0f23c93

chradek mentioned this pull request Oct 27, 2019

[storage-queue] add missing jsdocs & export types that were missed out #5861

Merged

chradek added 4 commits October 27, 2019 11:53

renames Retry* to StorageBlobRetry*

26218bf

renames BrowserPolicy

6885a67

Revert "renames BrowserPolicy"

1b71fa9

This reverts commit 6885a67.

Revert "renames Retry* to StorageBlobRetry*"

323182a

This reverts commit 26218bf.

HarshaNalluru added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Oct 27, 2019

HarshaNalluru reviewed Oct 27, 2019

View reviewed changes

ramya-rao-a reviewed Oct 27, 2019

View reviewed changes

ramya-rao-a approved these changes Oct 27, 2019

View reviewed changes

ramya-rao-a changed the title ~~[storage-blob] add jsdocs~~ [storage-blob] add missing jsdocs & export types that were missed out Oct 27, 2019

bterlson approved these changes Oct 28, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into storage-api-docs

5c4e708

XiaoningLiu approved these changes Oct 28, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into pr/chradek/5857

efd03bb

chradek merged commit af6345e into Azure:master Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[storage-blob] add missing jsdocs & export types that were missed out #5857

[storage-blob] add missing jsdocs & export types that were missed out #5857

chradek commented Oct 26, 2019

ramya-rao-a Oct 27, 2019

chradek Oct 27, 2019

ramya-rao-a Oct 27, 2019

ramya-rao-a Oct 27, 2019

ramya-rao-a Oct 27, 2019

bterlson Oct 27, 2019

chradek Oct 27, 2019

ramya-rao-a Oct 27, 2019

ramya-rao-a Oct 27, 2019

bterlson Oct 27, 2019

chradek Oct 27, 2019

ramya-rao-a Oct 27, 2019

ramya-rao-a Oct 27, 2019 •

edited

Loading

ramya-rao-a Oct 27, 2019

ramya-rao-a Oct 27, 2019

chradek Oct 27, 2019

ramya-rao-a Oct 27, 2019

HarshaNalluru Oct 27, 2019 •

edited

Loading

chradek Oct 27, 2019

HarshaNalluru Oct 27, 2019

HarshaNalluru Oct 27, 2019

jeremymeng commented Oct 27, 2019 via email

chradek commented Oct 27, 2019

bterlson commented Oct 27, 2019 •

edited

Loading

chradek commented Oct 27, 2019

HarshaNalluru Oct 27, 2019

ramya-rao-a Oct 27, 2019

chradek Oct 27, 2019

jeremymeng commented Oct 27, 2019

XiaoningLiu left a comment

[storage-blob] add missing jsdocs & export types that were missed out #5857

[storage-blob] add missing jsdocs & export types that were missed out #5857

Conversation

chradek commented Oct 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a Oct 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru Oct 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremymeng commented Oct 27, 2019 via email

chradek commented Oct 27, 2019

bterlson commented Oct 27, 2019 • edited Loading

chradek commented Oct 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremymeng commented Oct 27, 2019

XiaoningLiu left a comment

Choose a reason for hiding this comment

ramya-rao-a Oct 27, 2019 •

edited

Loading

HarshaNalluru Oct 27, 2019 •

edited

Loading

bterlson commented Oct 27, 2019 •

edited

Loading