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

Deprecate tools inheritance #224

Merged
merged 12 commits into from
Apr 30, 2021
Merged

Deprecate tools inheritance #224

merged 12 commits into from
Apr 30, 2021

Conversation

chrieke
Copy link
Contributor

@chrieke chrieke commented Apr 20, 2021

Fully deprecates the Tools module inheritance in the other classes (after deprecation process). The tools functions will only be available from the up42 import object.

Items:

  • Ran test & live-tests
  • Implemented (new) unit tests
  • Removed credentials
  • Removed argument pointing to staging
  • Updated documentation

For release:

  • Bumped version
  • Added changelog
  • Updated announcement banner

@@ -219,7 +219,7 @@ def _request(
data: Union[dict, list] = {},
querystring: dict = {},
return_text: bool = True,
) -> Union[str, dict, requests.Response]:
): # Union[str, dict, requests.Response]:
Copy link
Contributor Author

@chrieke chrieke Apr 29, 2021

Choose a reason for hiding this comment

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

Not exactly sure why lots of ambiogus response type checks were only triggered after the class inheritance removal, but had to deactivate to not add type assertions in every function only so that mypy understands the return type. The current potential return types of our API are not ideal (besides json, a few responses give text or e.g. quicklook data chunks). This will at some point be adressed at API level.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was looking at the problem before and could not make sense of it. Don't think it's critical, but it would surely be good if that would be resolved some time in the future; as this repo is open source people will wonder. Maybe create a tech ticket?

Copy link
Contributor

@markusuwe markusuwe left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -219,7 +219,7 @@ def _request(
data: Union[dict, list] = {},
querystring: dict = {},
return_text: bool = True,
) -> Union[str, dict, requests.Response]:
): # Union[str, dict, requests.Response]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Was looking at the problem before and could not make sense of it. Don't think it's critical, but it would surely be good if that would be resolved some time in the future; as this repo is open source people will wonder. Maybe create a tech ticket?

@chrieke chrieke merged commit a9e7bc0 into master Apr 30, 2021
@chrieke chrieke deleted the deprecate_tools_inheritance branch April 30, 2021 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants