Skip to content
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

[Service Bus] Change getter of boolean values to isXyz() #15890

Merged
merged 7 commits into from
Oct 29, 2020

Conversation

YijunXieMS
Copy link
Contributor

@YijunXieMS YijunXieMS commented Oct 2, 2020

It's customary to use isXyz() for a boolean value getter than getXyz(), as suggested in issue #14139 . So This PR suggests the following and other similar changes to the public APIs.

enableBatchedOperations() -> isBatchedOperationsEnabled()
enablePartitioning() -> isPartitioningEnabled()
deadLetteringOnMessageExpiration -> isDeadLetteringOnMessageExpiration()
requiresDuplicateDetection() -> isDuplicateDetectionRequired()
supportsOrdering() -> isOrderingSupported()

ServiceBusAdministrationClient has getQueueExists(String queueName) and other similar methods. This get method name kept because it implies an HTTP get operation. .Net calls it QueueExists.

Closes #14139

@YijunXieMS YijunXieMS added Service Bus Client This issue points to a problem in the data-plane of the library. APIChange This PR contains an addition or change to the API signature and must be reviewed by an architect. labels Oct 2, 2020
@YijunXieMS YijunXieMS added this to the [2020] October milestone Oct 2, 2020
@YijunXieMS YijunXieMS requested a review from yvgopal as a code owner October 2, 2020 16:18
@YijunXieMS YijunXieMS self-assigned this Oct 2, 2020
Copy link
Member

@conniey conniey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ramya-rao-a
Copy link
Contributor

The changes in this PR are not necessarily applicable only to Java. The other languages we are working on for Service Bus should be considered as well.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My conclusions after multiple discussions on this subject:

We are not too keen on being consistent between the "Enable" and "Requires" prefixes as matching with what goes over the wire trumps the small win of being consistent with the prefixes. This would not hold true for cases where the name is way too bad in the first place, but we do not find any such instances for Service Bus at the moment.

Unlike other languages, Java has getters and setters with get/set prefixes. For boolean return types, is is preferred instead of get. Since most of the properties do not read well if we just added is prefix, we will be taking the liberty to reword the names of these properties to help with readability. These will not match exactly with what goes over the wire, but will have similar words, so that should not be a problem. The win to readability trumps for Java due to the necessity to have separate prefixes for the getters and setters.

My mistake was to assume that the efforts in this PR also included in improving names in general and so would affect other languages.

Having said this, @JonathanGiles and I noticed that we are not updating the setters in this PR. One would expect the setter and getter names to differ only in the prefix i.e. is vs set. Can we have the setters updated here as well?

@YijunXieMS
Copy link
Contributor Author

YijunXieMS commented Oct 23, 2020

boolean getters and setters suggestion.
@ramya-rao-a @JonathanGiles Does this look good to you?

attribute name getter (is) setter
enableBatchedOperations isBatchedOperationsEnabled setBatchedOperationsEnabled
enablePartitioning isPartitioningEnabled setPartitioningEnabled
requiresDuplicateDetection isDuplicateDetectionRequired setDuplicateDetectionRequired
requiresSession isSessionRequired setSessionRequired
deadLetteringOnMessageExpiration isDeadLetteringOnMessageExpiration setDeadLetteringOnMessageExpiration
deadLetteringOnFilterEvaluationExceptions isDeadLetteringOnFilterEvaluationExceptions setDeadLetteringOnFilterEvaluationExceptions
supportOrdering isOrderingSupported setOrderingSupported
requiresPreprocessing isPreprocessingRequired setPreprocessingRequired

@ramya-rao-a ramya-rao-a dismissed their stale review October 28, 2020 19:18

Dismissing my stale review

@YijunXieMS YijunXieMS merged commit b547dd3 into Azure:master Oct 29, 2020
@YijunXieMS YijunXieMS deleted the sb_bool branch October 29, 2020 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APIChange This PR contains an addition or change to the API signature and must be reviewed by an architect. Client This issue points to a problem in the data-plane of the library. Service Bus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change names of booleans to use "is"
5 participants