Skip to content

Conversation

@YijunXieMS
Copy link
Contributor

Enable pylint and mypy check in CI and fix all errors reported by them.

@YijunXieMS YijunXieMS added Event Hubs Client This issue points to a problem in the data-plane of the library. labels May 7, 2020
@YijunXieMS YijunXieMS self-assigned this May 7, 2020
@YijunXieMS
Copy link
Contributor Author

YijunXieMS commented May 7, 2020

/azp run python - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

"""
return super(ServiceBusSessionReceiver, cls).from_connection_string(conn_str, **kwargs)
constructor_args = super(ServiceBusSessionReceiver, cls)._from_connection_string(conn_str, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

I believe it would have done the right behavior even in the single-call form, since it was using "cls" to initialize the object regardless; but if we want to decouple it further, let's at least name this inner function (_from_connection_string) something more useful than just a magic hidden callable of the same structure. Perhaps "_get_constructor_args_from_connection_string" (Yes, verbose, but very clear ><)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. The single-call form is right. It's a false alarm of mypy. I'll change it back and suppress that mypy error. The original way is neat.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

" the entity name in parameter is {}.".format(entity_in_conn_str, entity_in_kwargs)
)

kwargs["fully_qualified_namespace"] = host

Choose a reason for hiding this comment

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

Last dumb nitpick: why not used a named tuple for this, if we're returning a structured and constrained set of values? (I've always had a dislike for arbitrary dicts because you get so little guidance on "what to expect/what's correct")

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@YijunXieMS YijunXieMS merged commit 91de6c7 into Azure:master May 16, 2020
@YijunXieMS YijunXieMS deleted the sb_lint branch May 16, 2020 01:58
iscai-msft added a commit that referenced this pull request May 18, 2020
…into feature/text_analytics_v3.0

* 'master' of https://github.com/Azure/azure-sdk-for-python:
  [text analytics] Update ta tests (#11461)
  Release azure mgmt hybridkubernetes (#11483)
  add ci to azure-mgmt-eventhub (#11459)
  [Service Bus] Enable pylint and mypy (#11316)
  update CODEOWNERS with smoke test owners (#11404)
  Sync eng/common directory with azure-sdk-tools repository (#11469)
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. Event Hubs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants