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

added isinstance checks to fix tox pyright errors #39780

Closed
wants to merge 7 commits into from

Conversation

chrisribe
Copy link
Member

@chrisribe chrisribe commented Feb 18, 2025

Description

I fixed some pyright errors that where showing up in builds.
Build error ref: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=4534605&view=logs&j=b70e5e73-bbb6-5567-0939-8415943fadb9&t=56d4e2e6-c43b-527c-bad7-234ebe7429dd&l=86

The changes where made to the following files, but I am not sure if these are generated or not.

  1. So if someone could confirm this is the proper way to fix these errors that would be great.
  2. Was not sure about the changelog format and version (needs double check)

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@chrisribe
Copy link
Member Author

Finally green all checks passed 💯

@@ -323,15 +323,27 @@ def _get_type_alias_type(module_name: str, alias_name: str):
return types[alias_name]


def _get_model(module_name: str, model_name: str):
def _get_model(module_name: str, model_name: typing.Union[str, typing.ForwardRef]) -> typing.Any:
Copy link
Member

Choose a reason for hiding this comment

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

Given these typing errors arise from the generated code, it is up to the code generator to fix them. We test our code generator against the latest versions of the type checkers we run, so it should generate type clean code for each client library. It looks like this library was last generated ~7 months ago. I suspect if we regenerate the code with the latest version of the generator that these errors will be cleaned up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @kristapratico for your comment. It prompted me to review the documentation again. I found it odd that I needed to modify these files, as the comments didn't indicate they were auto-generated.

I now understand that these files need to be rebuilt locally using the tsp-client and the latest azure-rest-api-specs for our project. As you mentioned, this hasn't been done in the last ~7 months, and I just picked this up. Apologies for the confusion.

Copy link
Member Author

@chrisribe chrisribe Feb 24, 2025

Choose a reason for hiding this comment

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

Ok seems I am stepping on toes here and a PR is already in review:
#38437

Dropping this PR request.
Sorry for the bother.

@chrisribe
Copy link
Member Author

Closing duplicate effort.
#38437

@chrisribe chrisribe closed this Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants