Skip to content

Conversation

@JaviCerveraIngram
Copy link
Collaborator

Added type hints in a Python 2 compatible way (by putting the types in comments following member var and method declarations).

This allows PyCharm's Code Inspect tool to capture access to invalid attributes, and vastly improves intellisense, by providing suggestions of available attributes when using the dot operator on a model.

JaviCerveraIngram and others added 30 commits March 4, 2019 12:50
# Conflicts:
#	README.md
#	connect/resource/base.py
#	example/example.py
#	tests/test_config.py
# Conflicts:
#	connect/models/asset.py
#	connect/models/fulfillment.py
@codecov-io
Copy link

codecov-io commented Mar 19, 2019

Codecov Report

Merging #22 into master will increase coverage by 2.63%.
The diff coverage is 94.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
+ Coverage   84.25%   86.88%   +2.63%     
==========================================
  Files          24       24              
  Lines         527      633     +106     
==========================================
+ Hits          444      550     +106     
  Misses         83       83
Impacted Files Coverage Δ
connect/resource/utils.py 100% <ø> (ø) ⬆️
connect/resource/fulfillment.py 69.69% <100%> (+0.94%) ⬆️
connect/models/parameters.py 100% <100%> (ø) ⬆️
connect/models/connection.py 100% <100%> (ø) ⬆️
connect/config.py 92.3% <100%> (+0.2%) ⬆️
connect/models/asset.py 100% <100%> (ø) ⬆️
connect/models/__init__.py 100% <100%> (ø) ⬆️
connect/models/server_error.py 92.85% <100%> (+1.94%) ⬆️
connect/resource/base.py 82.5% <100%> (+1.41%) ⬆️
connect/models/contact.py 100% <100%> (ø) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5facb7f...e4faeb4. Read the comment docs.

products=None,
file=None
):
# type: (str, str, Union[str, List[str]], str) -> None
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, better to use alternate syntax for multi-parameter functions, where hint comment placed right after parameter?
api_url=None, # type: str
api_key=None, # type: str
...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you feel that's more readable, I'll change it, the version you propose is perfectly fine and PyCharm understands it. I just used the other one because that's how PyCharm autogenerates the hints when you select 'Add type hints for function'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed.

return ActivationTemplateResponse(template_id=pk)

def list(self):
# type: () -> Any
Copy link
Contributor

Choose a reason for hiding this comment

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

more or less pointless type-hinting (there and in similar places)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True. I'll check those and fix them. This one should probably be # type: () -> List[BaseModel] on BaseResource, and a list of specific BaseModel subclasses on derived resources.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


@property
def headers(self):
# type: () -> dict
Copy link
Contributor

Choose a reason for hiding this comment

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

why not

type: () -> Dict[str, str]

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know why :) I'll fix this one and check similar ones.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


@function_log
def update_parameters(self, pk, params):
# type: (str, list) -> str
Copy link
Contributor

Choose a reason for hiding this comment

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

(str, List[Param]) -> str ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, better like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@vgrebenschikov vgrebenschikov merged commit 1ead321 into cloudblue:master Mar 22, 2019
@JaviCerveraIngram JaviCerveraIngram deleted the type-hints branch March 25, 2019 08:05
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.

3 participants