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

[ServiceBus]remove topic parameter object settability #14116

Conversation

KieranBrantnerMagee
Copy link
Member

On request from cross-language SDK leads, disabling these Union types to encourage clarity of what can be passed here, as well as reduce ambiguity from "what object does this act on."

@KieranBrantnerMagee KieranBrantnerMagee added Service Bus Client This issue points to a problem in the data-plane of the library. labels Sep 29, 2020
@KieranBrantnerMagee KieranBrantnerMagee added this to the [2020] October milestone Sep 29, 2020
@KieranBrantnerMagee KieranBrantnerMagee self-assigned this Sep 29, 2020
@KieranBrantnerMagee
Copy link
Member Author

/azp run python - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@KieranBrantnerMagee
Copy link
Member Author

/azp run python - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@KieranBrantnerMagee
Copy link
Member Author

/azp run python - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -603,11 +594,11 @@ def get_subscription_runtime_properties(self, topic, subscription_name, **kwargs
entry.title, entry.content.subscription_description)
return subscription

def create_subscription(self, topic, name, **kwargs):
# type: (Union[str, TopicProperties], str, Any) -> SubscriptionProperties
def create_subscription(self, topic_name, name, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Maybe now that we've suffixed all the others, should we update name to subscription_name?

Copy link
Contributor

@yunhaoling yunhaoling Oct 1, 2020

Choose a reason for hiding this comment

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

(I found myself liking the pattern of prefix <entity>_name for clarity. But if we want the change, create_queue, create_topic, create_rule also need to be updated)

Copy link
Member Author

Choose a reason for hiding this comment

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

I like and agree with this change, but both because of coming-to-wire for CC and, less-lazily-and-more-professionally, to keep this PR scope constrained, do folks mind if I put that as an issue for our next sprint?

Copy link
Member Author

Choose a reason for hiding this comment

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

OOB folks concurred this is fine, issue created for nov milestone.

annatisch
annatisch previously approved these changes Oct 1, 2020
Copy link
Member

@annatisch annatisch left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

Fix doctype that had old name, make changelog note more precise.

Co-authored-by: Adam Ling (MSFT) <adam_ling@outlook.com>
@KieranBrantnerMagee
Copy link
Member Author

/azp run python - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@KieranBrantnerMagee
Copy link
Member Author

/azp run python - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@KieranBrantnerMagee KieranBrantnerMagee merged commit 6e5caf2 into Azure:master Oct 3, 2020
iscai-msft added a commit that referenced this pull request Oct 7, 2020
…into fr-business-cards

* 'master' of https://github.com/Azure/azure-sdk-for-python: (71 commits)
  move the environment prep above the tooling that needs it (#14246)
  Increment version for appconfiguration releases (#14245)
  Azure Communication Service - Phone Number Administration (#14237)
  [text analytics] fix query param in cli call to get endpoint (#14243)
  Resolve Failing Documentation Build for azure-mgmt-core (#14239)
  Add code reviewers (#14229)
  [ServiceBus] make amqp_message properties read-only (#14095)
  [ServiceBus]remove topic parameter object settability (#14116)
  app config owner (#12986)
  [KeyVault] Handle Role Definition UUID Name Internally (#14218)
  Increment version for storage releases (#14224)
  Update Key Vault changelogs for October release (#14226)
  [ServiceBus] CI Test hotfixes (#14195)
  [text analytics] regen TA with GA autorest (#14215)
  [Storage][STG74]ChangeLog (#14192)
  fixes python 2.7 issue with unicode and strings again! (#14216)
  Feature/storage stg74 (#14175)
  Update communication pacakges to version b2 (#14209)
  [KeyVault] Add Status Methods to Query Backup and Restore Operations (#14158)
  Update buffered sender (#13851)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants