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 type annotations and mypy compliance #225

Merged

Conversation

abought
Copy link
Contributor

@abought abought commented May 18, 2017

🦑 Ready for review, but watch for sea monsters (my first WB PR)

Ticket

https://openscience.atlassian.net/browse/SVCS-351

Purpose

  • Add type annotations to assist with debugging (core classes and some, but not all, providers)
  • Provide an option (currently not on by default) to use mypy, a linter for gradual type hinting- catches weird usages of methods

Notes for reviewers

To try this out, run inv mypy.

In the future this can be used in Travis CI, but is currently hidden behind a flag by default:
inv test --types

Many of these types are a "best guess" and subject to evolve in the future. Python types are informational only and do not affect runtime, but tools such as mypy (and pycharm!) support automatic linting to catch errors during development.

I am new to waterbutler. If there is a better type to use, let me know. The ignore settings in mypy may be too aggressive and are worth revisiting in the future.

Specific rules that we ignore

Mypy does not appear to support selective ignoring of certain rules. Therefore, ignore statements are scattered throughout the code more than I would wish. See notes and rationale:

  • Mypy doesn't seem to do very well with functions that can return more than one type of thing. (so basically every call to metadata gets ignored) See:
    https://github.com/python/mypy/issues/1693
    • This could be resolved using dependent types / @overload, if we some day refactor so that files and folders are represented via different classes.
  • Method call signatures or return types are not consistent with parent classes. We routinely violate the Liskov substitution principle, of necessity as there is no uniform abstraction for all providers. See: https://github.com/python/mypy/issues/1237
    • There are a lot of ignores for this. It's terrible; I'm sorry.

Summary of changes

  • Add type annotations to code
  • Update docstrings
  • Update method signatures that did not reflect the common usages across all subclasses

Testing notes

In theory this is a pure developer facing change. One or two minor changes were made to method call signatures.

Recommend watching for any new errors during common operations. I have done light manual testing consisting of uploads/downloads/renames/cross provider moves (osf storage to google drive).

@abought abought changed the title [WIP] Refactor WaterbutlerPath object for better directory/ID abstraction [WIP] Add type annotations and mypy compliance May 22, 2017
abought added a commit to abought/waterbutler that referenced this pull request May 22, 2017
+ improved call signature stubs for base provider class

Many errors are skipped due to present limitations, esp Liskov issues
See CenterForOpenScience#225

[#SVCS-340]
[ci skip]
Copy link
Member

@felliott felliott left a comment

Choose a reason for hiding this comment

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

Looks good, @abought! One minor nitpick per our poorly-advertised CONTRIBUTING.md.

import json
import os
import typing
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Per our CONTRIBUTING.md, imports should be split into three groups (core, external, internal) and each group ordered by line length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 3705f2a

I haven't yet manually tested this; is there a page with suggestions on common operations to verify?

Some tests are passing locally but failing on travis; still investigating why. In particular, as part of the cleanup of abc stub classes, I added async keyword to a few methods of the base class (when all child classes were using it). Want to make sure this doesn't cause unintended side effects.

@abought abought force-pushed the feature/svcs340-waterbutlerpath branch from 233979d to 55ed70c Compare May 24, 2017 14:54
abought added 11 commits May 26, 2017 10:18
then start on abstract implementation

[#SVCS-340]
[ci skip]
+ improved call signature stubs for base provider class

Many errors are skipped due to present limitations, esp Liskov issues
See CenterForOpenScience#225

[#SVCS-340]
[ci skip]
Add type annotations to metadata and path objects

[#SVCS-351]
 a set of conventions (sometimes arbitrary) about how to write
 code for that project."

[#SVCS-351]
@abought abought force-pushed the feature/svcs340-waterbutlerpath branch from 3705f2a to a824ab9 Compare May 26, 2017 14:49
@abought
Copy link
Contributor Author

abought commented May 26, 2017

Initial manual testing ok. Merge conflicts with googledrive refactor; I will update and revalidate before next week.

@abought abought force-pushed the feature/svcs340-waterbutlerpath branch from b980aa3 to e3fc183 Compare May 30, 2017 02:46
@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 75.6% when pulling e3fc183 on abought:feature/svcs340-waterbutlerpath into 753bdd2 on CenterForOpenScience:develop.

@abought
Copy link
Contributor Author

abought commented May 30, 2017

Merge conflicts resolved and basic retesting done. Let me know if there are additional checks requested!

@abought abought changed the title [WIP] Add type annotations and mypy compliance Add type annotations and mypy compliance May 30, 2017
@felliott felliott merged commit e3fc183 into CenterForOpenScience:develop May 31, 2017
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