-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[AppConfig] mypy fixes #18858
[AppConfig] mypy fixes #18858
Conversation
@@ -228,7 +226,7 @@ def get_configuration_setting( | |||
etag="*", | |||
match_condition=MatchConditions.Unconditionally, | |||
**kwargs | |||
): # type: (str, Optional[str], Optional[str], Optional[MatchConditions], **Any) -> ConfigurationSetting | |||
): # type: (str, Optional[str], Optional[str], Optional[MatchConditions], **Any) -> Optional[ConfigurationSetting] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems odd to me. Is there a way that you can call this method and it will return None
instead of a ConfigurationSetting? This could almost be considered a breaking change if we used to document this as always returning a ConfigurationSetting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch
Hello @seankane-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
etag="*", # type: Optional[str] | ||
match_condition=MatchConditions.Unconditionally, # type: Optional[MatchConditions] | ||
**kwargs # type: Any | ||
): # type: (...) -> Union[None, ConfigurationSetting] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still seems odd to me, but if it's the expected behavior then I think we should probably update the :rtype:
in the docstring and perhaps explain in :return:
what case None
could be returned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I didn't see the auto-merge tag, sorry
…into get_testserver_working * 'master' of https://github.com/Azure/azure-sdk-for-python: (55 commits) [AppConfig] mypy fixes (Azure#18858) [translation] renames (Azure#19285) [Key Vault] Update metadata for release (Azure#19286) [AppConfig] enable samples (Azure#18857) KeyVaultBackupOperation -> KeyVaultBackupResult (Azure#19284) renaming (Azure#19280) [Key Vault] Custom polling method for admin backup client (Azure#19204) [textanalytics] add ARM template + run samples in CI (Azure#19270) removed example from description (Azure#18781) [Key Vault] Update READMEs with RBAC information (Azure#19275) Sync eng/common directory with azure-sdk-tools for PR 1688 (Azure#19272) update all ubuntu vmImage to 20.04 (Azure#19227) add base class for feedback (Azure#19265) Enable tests for integration samples (Azure#18531) docs: fix a few simple typos (Azure#19127) Increment package version after release of azure-eventgrid (Azure#19197) Increment package version after release of azure-monitor-query (Azure#19208) earliest versions of communication identity tests are set up improperly. skip 1.0.0 in regression testing (Azure#19258) Run mypy in azure-keyvault-administration CI (Azure#19246) [text analytics] change return type of analyze actions to list of list (Azure#18994) ...
Yuliu2/add purview configuration (Azure#18858) * Defining the APIs for the new NSP resource * Add PurviewConfiguration property in ADF Add PurviewConfiguration property in ADF * Fix discriminator Fix discriminator * Remove discriminator Remove discriminator * Update datafactory.json Make it consistent with Synapse * Rename PurviewConfiguration Co-authored-by: Hari Prasad Perabattula <[email protected]> Co-authored-by: Hari Prasad Perabattula <[email protected]>
closes #18540