Skip to content

Remove ".py" report#204

Closed
ritwik12 wants to merge 2 commits intogoogle:masterfrom
ritwik12:master
Closed

Remove ".py" report#204
ritwik12 wants to merge 2 commits intogoogle:masterfrom
ritwik12:master

Conversation

@ritwik12
Copy link
Copy Markdown
Contributor

Part of #111

@rchen152
Copy link
Copy Markdown
Contributor

Thanks! It looks like pytype/tools/analyze_project/pytype_runner_test.py needs to be updated.

@rchen152
Copy link
Copy Markdown
Contributor

Actually, there's one more place in pytype that probably needs to be changed: https://github.com/google/pytype/blob/master/pytype/tools/analyze_project/main.py#L77. If import_graph.unreadable files is now going to include all files regardless of extension, then we shouldn't report an error unless an unreadable file is a Python file.

I think something like the following would work:

try:
  ...
  unreadable_inputs = conf.inputs & import_graph.unreadable_files
  unreadable_python_inputs = {file for file in unreadable_inputs if file.endswith('.py')}
      assert not unreadable_python_inputs, '\n  '.join(
          ['Unparseable in Python %s:' % conf.python_version] +
        sorted(unreadable_python_inputs))
except ...:
  ...
conf.inputs -= unreadable_inputs

@ritwik12
Copy link
Copy Markdown
Contributor Author

@rchen152 Thanks for all your inputs, I guess it should work now.

@rchen152
Copy link
Copy Markdown
Contributor

Thanks for sending the additional PR. For this one, the failing test still needs to be updated. It looks like the broken test cases are TestYieldSortedModules.test_non_py_dep and TestGetModuleAction.test_skip in pytype/tools/analyze_project/pytype_runner_test.py. Since they're testing behavior that this change is removing, I think you can just delete them.

@ritwik12 ritwik12 mentioned this pull request Dec 15, 2018
@ritwik12
Copy link
Copy Markdown
Contributor Author

@rchen152 I hope #207 will do it.

@ritwik12
Copy link
Copy Markdown
Contributor Author

@rchen152 I guess it will keep on failing until #207 is merged.

@rchen152
Copy link
Copy Markdown
Contributor

Could you copy the changes from #207 into this PR instead? It's better to have the test removal done together with the motivating change; also, if they're submitted separately, I think you'd have to rebase this PR on top of the master branch.

rchen152 pushed a commit that referenced this pull request Dec 17, 2018
Few Changes in try block in continuation with #204

Resolves #205

PiperOrigin-RevId: 225883515
@ritwik12
Copy link
Copy Markdown
Contributor Author

@rchen152 Sure, I will.

@ritwik12
Copy link
Copy Markdown
Contributor Author

@rchen152 It's done.

Copy link
Copy Markdown
Contributor

@rchen152 rchen152 left a comment

Choose a reason for hiding this comment

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

Thanks, looks good. Again, please don't merge this PR; it'll be closed automatically once this change is imported and re-exported.

@ritwik12
Copy link
Copy Markdown
Contributor Author

@rchen152 Sure, Thanks!!

rchen152 pushed a commit that referenced this pull request Dec 18, 2018
Part of #111

Resolves #204

PiperOrigin-RevId: 226035107
rchen152 added a commit that referenced this pull request Dec 18, 2018
@ritwik12
Copy link
Copy Markdown
Contributor Author

@rchen152 Thanks!!

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