Skip to content

Enhance mypy support. #126

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

Closed
wants to merge 5 commits into from
Closed

Enhance mypy support. #126

wants to merge 5 commits into from

Conversation

roberthoenig
Copy link
Collaborator

This

  • runs mypy and lint in a single travis job (so we have five jobs again, the maximum that can be parallely executed)

  • adds support for adding directories to the lister exclude list (needed for excluding zulip_bots/zulip_bots/bots

  • runs mypy for each package fully and separately

  • excludes all files that need to be fixed.

Btw I noticed that we have a lister duplicate; one in tools and one in tools/server_lib. Which one should we keep? I vote for the server_lib one and move pep8 and custom_check.py there.

tools/lister.py Outdated
if os.path.isdir(normed_path):
exclude_abspaths += [os.path.normpath(os.path.join(repository_root, fpath)) for fpath in list_files(targets=[normed_path])]
else:
exclude_abspaths.append(normed_path)
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this didn't already work? This should work:

        if any(absfpath == expath or absfpath.startswith(expath + '/')                               
               for expath in exclude_abspaths):                                                      
            continue                                                                                 

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From my tests and the comment in the code, excludes_paths didn't exclude directories - we might want to make the same change to zulip?

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 the comments are inaccurate. Looking at the history, we used to have contrib_bots/bots in the exclude paths in the server repo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh right - I see the issue then, it's Windows paths:

if any(absfpath == expath or absfpath.startswith(expath + '/')
               for expath in exclude_abspaths):
            continue

hardcodes the delimiter, so it didn't work for the Windows files. Will update that snippet instead.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, yeah, and that fix we should contribute to the main repo too.

.travis.yml Outdated
env: TEST_SUITE=lint
- python: "3.6"
env: TEST_SUITE=run-mypy
env: TEST_SUITE=test-static
Copy link
Member

Choose a reason for hiding this comment

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

maybe TEST_SUITE=static-analysis? I feel like we probably want this to be clearer than "test-static" as to what it is.

tools/run-mypy Outdated
if rc:
sys.exit(rc)
else:
print("There are no files to run mypy on.")
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 we want it to check all 3 packages even if there are errors in the first one; so we should stored a failed variable instead of existing immediately on the first error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I was not sure about that bit.

@timabbott
Copy link
Member

I posted a few comments; thanks for working on this @derAnfaenger!

@roberthoenig
Copy link
Collaborator Author

Updated. Includes a new commit to remove the duplicate lister.py

@timabbott
Copy link
Member

This is good enough; I made one change to the last commit to remove the |= behavior (it's too hard to read and means the wrong thing, since it's a bitwise or!), and merged.

I'm not totally convinced by the use of pathlib.PurePath; why do we need that? It feels like use of overly complex Python features for something simple :). But we can address that in a follow-up commit.

@roberthoenig
Copy link
Collaborator Author

roberthoenig commented Sep 28, 2017

I used pathlib.PurePath since it makes extracting the first segment of a path very easy. pathlib.Path has the same functionality but is more heavy. os.path doesn't offer any comparable method. The easiest non-path alternative I can think of is extracting the beginning of the string until the first separator.

@timabbott
Copy link
Member

OK, I guess that's fine.

@neiljp
Copy link
Contributor

neiljp commented Sep 28, 2017

pathlib is 3.4+; are tools generally 3+ only?

@roberthoenig
Copy link
Collaborator Author

The mypy package. is python3 only.

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.

4 participants