Skip to content

Conversation

ilinum
Copy link
Collaborator

@ilinum ilinum commented Jun 26, 2017

No description provided.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Looks fine! Amazing what such a simple change found. :-)

@gvanrossum
Copy link
Member

However you need to figure out why the Windows tests are failing. Probably something with path separators (it always is, except when it's LF vs. CRLF :-).

ilinum added 2 commits June 27, 2017 10:17
As it turns out, path to css on Windows was broken, which means
--html-report was not useful on Windows at all
@ilinum
Copy link
Collaborator Author

ilinum commented Jun 27, 2017

As it turns out, the path to the CSS file on windows was wrong. It was trying to use the native filepath. However, on Windows the \ would get converted to %5C, so path to CSS file would look like ..%5Cmypy-html.css, which is wrong.

What this means is that css was not working on windows and lines in html report did not have colors. This is fixed now in this PR.

So you're right, it was with directory separators :)

@ilinum
Copy link
Collaborator Author

ilinum commented Jun 28, 2017

I realized that this PR does not address reporting of dead code in --any-expr-report.
I don't think it's ready to be merged until this is done.

@ilinum
Copy link
Collaborator Author

ilinum commented Jun 29, 2017

This PR is ready for review!

Now --any-expr-report counts each line of dead code as one Any.

Copy link
Collaborator

@ddfisher ddfisher left a comment

Choose a reason for hiding this comment

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

Feel free to merge after fixing nits if you're confident that the updated comments are excellent.

mypy/stats.py Outdated
TYPE_PRECISE = 1
TYPE_IMPRECISE = 2
TYPE_ANY = 3
TYPE_UNANALYZED = 1 # type of unchecked code (different from any)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment isn't fully clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

mypy/stats.py Outdated

def visit_class_def(self, o: ClassDef) -> None:
# this method is overridden because we don't want to visit base_type_exprs
# base_type_exprs are expressions without a type, which causes them to appear as Any
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you reword this comment? I don't understand what's going on here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, hopefully, should be better now.

mypy/stats.py Outdated
if not t:
# if an expression does not have a type, it is often due to dead code
# we should not keep track of the number of these we encounter because there can be
# an unanalyzed value on a line with other analyzed expressions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use standard sentence capitalization/punctuation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok!

# we should not keep track of the number of these we encounter because there can be
# an unanalyzed value on a line with other analyzed expressions
self.record_line(self.line, TYPE_UNANALYZED)
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: probably better to change the if isinstance(t, Instance) to an elif below instead of this early return.

Copy link
Collaborator Author

@ilinum ilinum Jul 5, 2017

Choose a reason for hiding this comment

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

I don't think that would work - there are two separate if-statements and I think it's easier to just address t possibly being None in the beginning and do an early return.

@ilinum ilinum merged commit 6c61c66 into python:master Jul 5, 2017
@ilinum ilinum deleted the report-any-unreachable branch July 5, 2017 22:54
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