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

Update mypy to 0.720 #6713

Closed
wants to merge 1 commit into from
Closed

Update mypy to 0.720 #6713

wants to merge 1 commit into from

Conversation

mkurnikov
Copy link
Contributor

@mkurnikov mkurnikov commented Jul 14, 2019

Refs #4748

@@ -59,9 +59,9 @@
except ImportError:
# typing's cast() isn't supported in code comments, so we need to
# define a dummy, no-op version.
def cast(typ, val):
def cast(typ, val): # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should add ignore here. (Also, in general I think we should try to avoid adding ignore if we can because otherwise it defeats the purpose of the typing.)

Copy link
Member

Choose a reason for hiding this comment

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

Also, if you need help finding ways to avoid ignore, feel free to ask.

return val
VersionInfo = None
VersionInfo = None # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Member

Choose a reason for hiding this comment

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

# `buf.value` is `unicode`, but methods return type is `str`.
# If we annotate here as unicode, lots of other places should be
# changed / ignored.
return buf.value # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

I think a better fix here might be to encode the path to str using sys.getfilesystemencoding(). Otherwise, we risk getting exceptions in calling code when trying to combine str and unicode if the path has non-ascii characters. As it is now these errors would be hard to prevent by the developer because the type annotation would be showing str, so a developer looking at it wouldn't know they have to encode it first.

@mkurnikov
Copy link
Contributor Author

@cjerdonek Could you finish it for me?
I don't quite understand workaround on the cast and VersionInfo, I tried them, locally I couldn't make it work.

@mkurnikov mkurnikov closed this Jul 15, 2019
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Aug 15, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants