Storage queue from Alan's PR #4036
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…olding of QueueServiceClients
* added servicebus entry to api-specs * generated v2015_08_01 of servicebus * generated v2018_01_01_preview of servicebus
* generated v2017_04_01 of servicebus * updated azure-arm-parent versioning
* generated v2015_06_01 of authorization * generated v2018_07_01_preview of authorization * generated v2018_09_01_preview of authorization * updated versioning for maven
* generated v2015_07_01 of monitor * generated v2016_03_01 of monitor * generated v2017_03_01_preview of monitor * generated v2017_04_01 of monitor * generated v2018_03_01 of monitor * generated v2018_04_16 of monitor * generated v2018_09_01 of monitor * formatted pom.xml * Update pom.xml
* added search entry to api-specs * generated v2015_02_28 of search
* added appconfiguration entry to api-specs * generated v2019_02_01_preview of appconfiguration
Adding API for blockUntil(desiredOperatopnStatus) refer to issue Azure#3931
Added Release notes + KV javadoc grouping update
* regenerated network with api version 2019_02_01 * updated parent versioning in pom * Update pom.xml
* Adding an example to authorize with AAD. * Fix broken links * Fixing link. * Sort links * Make links relative * Review changes.
* Initial prototype for paged flux * Update unit tests and javadocs * Remove unused imports * Updated to use paged response instead of continuation token * Update javadocs * Supplier and function * Update javadocs, add code snippets and more unit tests * Undo package-info changes * Undo package-info changes * Update javadocs * Add jsr dependency * Remove extra blank line
JonathanGiles
requested changes
Jun 29, 2019
pom.client.xml
Outdated
| @@ -523,6 +523,7 @@ | |||
| -snippetpath ${project.basedir}/eventhubs/client/azure-eventhubs/src/samples/java | |||
| -snippetpath ${project.basedir}/keyvault/client/keys/src/samples/java | |||
| -snippetpath ${project.basedir}/keyvault/client/secrets/src/samples/java | |||
| -snippetpath ${project.basedir}/storage/client/queue/src/samples/java/javadoc | |||
There was a problem hiding this comment.
There shouldn't be javadoc at the end of this line
storage/client/pom.xml
Outdated
| <configuration> | ||
| <failOnError>false</failOnError> | ||
| </configuration> | ||
| </plugin> |
There was a problem hiding this comment.
These changes can all be removed - it is centrally managed now.
There was a problem hiding this comment.
Checkstyle should be removed too, right?
There was a problem hiding this comment.
Yes, all configuration of these features is centralised
| * .endpoint(endpoint) | ||
| * .queueName(queueName) | ||
| * .buildAsync(); | ||
| * </pre> |
There was a problem hiding this comment.
This code snippet should be in an external .java file like other code snippets.
| * <pre> | ||
| * client.create(Collections.singletonMap("queue", "metadataMap")) | ||
| * .subscribe(response -> System.out.printf("Create completed with status code %d", response.statusCode())); | ||
| * </pre> |
There was a problem hiding this comment.
Make sure all code snippets are externalised - I won't keep mentioning this as I get further down the PR.
| * | ||
| * <p>Delete a queue</p> | ||
| * | ||
| * @codesnippet com.azure.storage.queue.queueAsyncClient.delete |
There was a problem hiding this comment.
None of these codesnippet blocks are valid - you are missing the opening and closing brace. Generate the JavaDoc and you'll see what I mean.
| * | ||
| * <p><strong>Code Samples</strong></p> | ||
| * | ||
| * <p>Get the properties of the queue</p> |
There was a problem hiding this comment.
Code snippet titles should be more bold than a simple paragraph block.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Code, Mono -> Flux
Whether merge in DequedMessage and PeekedMessage? No merge. Confirmed from Alan.
Code, getURL() or others? File an issue
Why 'Azure' in AzureQueueStorageBuilder? Already renamed in code.
connectionString() or credential() in builder patter
Code Is there ever a time a user would want to mutate or instantiate this class? To me it would be reasonable to make this an immutable class? It is from autogeneration. Will sync with Jianghao after blob change.
Code POJO or Fluent. Will do with POJO
Code This class API and the name of the class are not obviously related. Is UpdatedMessage really the right name here? This data model is used in QueueAsyncClient for updateMessage API, so the name is by design.