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

Add types to all of parameter #1769

Merged
merged 13 commits into from
Oct 22, 2019

Conversation

jenshnielsen
Copy link
Collaborator

There are a number of less than idea issues here that makes it tricky and requires workarounds

def __exit__(self, typ, value, traceback):
def __exit__(self, typ, # type: ignore[no-untyped-def]
value,
traceback) -> None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could type this as given in https://github.com/python/typeshed/blob/master/stdlib/2and3/contextlib.pyi#L23 and https://github.com/python/typeshed/blob/master/stdlib/3/types.pyi#L235 but that does not seem all that useful. If there was a nice way to get these types from the typeshead without re implementing that would help

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah....

@codecov
Copy link

codecov bot commented Oct 18, 2019

Codecov Report

Merging #1769 into master will decrease coverage by <.01%.
The diff coverage is 92%.

@@            Coverage Diff             @@
##           master    #1769      +/-   ##
==========================================
- Coverage   67.44%   67.44%   -0.01%     
==========================================
  Files         147      147              
  Lines       18471    18486      +15     
==========================================
+ Hits        12457    12467      +10     
- Misses       6014     6019       +5

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

( left some minor comments )

def __exit__(self, typ, value, traceback):
def __exit__(self, typ, # type: ignore[no-untyped-def]
value,
traceback) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah....

qcodes/instrument/parameter.py Outdated Show resolved Hide resolved
qcodes/instrument/parameter.py Outdated Show resolved Hide resolved
qcodes/instrument/parameter.py Outdated Show resolved Hide resolved
qcodes/instrument/parameter.py Outdated Show resolved Hide resolved
qcodes/instrument/parameter.py Outdated Show resolved Hide resolved
qcodes/instrument/parameter.py Outdated Show resolved Hide resolved
@@ -2075,40 +2132,48 @@ def _multiplier(self, multiplier: Union[int, float, Parameter]):

# Division of the scaler
@property
def division(self):
def division(self) -> float: # type: ignore[return]
Copy link
Contributor

Choose a reason for hiding this comment

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

why ignore return if the type is specified via casting? or am i misunderstanding something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Due to the mypy bug mentioned above. Remember that unused ignores is an error, so you can be sure this is needed anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is if mypy ever resolves the bug this will be an error and all to do is to remove this ignore comment

qcodes/instrument/parameter.py Outdated Show resolved Hide resolved
qcodes/instrument/parameter.py Outdated Show resolved Hide resolved
@jenshnielsen jenshnielsen merged commit 611d9ea into microsoft:master Oct 22, 2019
@jenshnielsen jenshnielsen deleted the mypy_force_parameter branch October 22, 2019 08:07
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.

None yet

2 participants