Add typing to signac.core.json. #313

bdice · 2020-03-21T15:27:50Z

Description

This is an experimental PR adding types to signac.core.json. This was created using MonkeyType and modified slightly. Adding type validation seems like a simple process.

To generate this, I installed monkeytype and then ran the following commands from the repository root:

# Collect type annotations based on runtime types
monkeytype run -m pytest -v
# List the modules with suggested annotations
monkeytype list-modules
# Show the type annotations it's suggesting
monkeytype stub signac.core.json
# Apply the suggested annotations
monkeytype apply signac.core.json
# Manually edit the suggested annotations to be more general
vim signac/core/json.py

We'd need to validate the results with mypy, I think.

Motivation and Context

Just wanted to try it out.

Types of Changes

New feature

¹The change breaks (or has the potential to break) existing functionality.

Checklist:

I am familiar with the Contributing Guidelines.
I agree with the terms of the Contributor Agreement.
My name is on the list of contributors.
My code follows the code style guideline of this project.
The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:

I have updated the API documentation as part of the package doc-strings.
I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
I have updated the changelog and added all related issue and pull request numbers for future reference (if applicable). See example below.

bdice · 2020-03-21T15:54:41Z

Adding mypy checking is probably a good idea. For example, it caught this bug:

signac/signac/contrib/filterparse.py

Line 59 in 232d018

print("Did you mean {}?".format(CAST_MAPPING_WARNING[x], file=sys.stderr))

Note that the parentheses are in the wrong places.

csadorf · 2020-03-23T10:43:21Z

@bdice Cool! Did you have a specific reason to start with that module?

bdice · 2020-03-24T14:43:21Z

@csadorf No specific reason to start there. @glotzerlab/signac-committers Can I get a couple opinions on this? Should we apply types? I think it is a good idea, and it is an automated safety check that we can use to ensure code quality going forward. It's a little bit like unit tests in that it can't/won't catch every potential problem but definitely reduces the "surface area" for potential problems.

csadorf · 2020-03-24T14:55:08Z

I am all in favor of slowly introducing typing to the code base. Is this already checked as part of the CI on this branch?

vyasr · 2020-03-25T19:33:38Z

We discussed adding types as part of glotzerlab/signac-flow#193. In that PR, I laid out what i think are the necessary preconditions for documenting parameter types. On that issue I pointed out that applying type hints could proceed independently of docs, and the basic CI setup for doc linting is already in place in this repo, so adding CI for types would be reasonable and not lead to any conflicting dev IMO.

codecov · 2020-03-28T00:57:13Z

Codecov Report

Merging #313 into master will decrease coverage by <.01%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master     #313      +/-   ##
==========================================
- Coverage   76.18%   76.18%   -0.01%     
==========================================
  Files          43       43              
  Lines        7076     7075       -1     
==========================================
- Hits         5391     5390       -1     
  Misses       1685     1685

Impacted Files	Coverage Δ
signac/contrib/project.py	`91.58% <ø> (ø)`	⬆️
signac/common/errors.py	`75% <ø> (-3.58%)`	⬇️
signac/contrib/filterparse.py	`83.87% <0%> (ø)`	⬆️
signac/common/host.py	`46.42% <100%> (ø)`	⬆️
signac/contrib/indexing.py	`75.59% <100%> (ø)`	⬆️
signac/syncutil.py	`82.8% <100%> (ø)`	⬆️
signac/core/json.py	`83.33% <100%> (+0.72%)`	⬆️
signac/core/h5store.py	`91% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update deb56fa...5f406e2. Read the comment docs.

mikemhenry · 2020-03-28T01:52:22Z

I love this!!!! I'm not sure what any downside would be. It can help us catch bugs + make our code easier to maintain.

bdice

I have annotated my PR with explanations for the benefit of the reader, since none of these changes are especially obvious without experience with type annotations / mypy.

bdice · 2020-03-28T03:12:38Z

signac/common/errors.py

@@ -22,9 +22,5 @@ class ExportError(Error, RuntimeError):
    pass


-class FileNotFoundError(Error, FileNotFoundError):


This seemed like an unused compatibility layer for Python 2 (which did not have FileNotFoundError). I removed it.

bdice · 2020-03-28T03:19:06Z

signac/core/h5store.py

+    cls = H5Store  # type: ignore
+    suffix = '.h5'  # type: ignore


Explanation for the ignore: These are overriding a default of None in the DictManager parent class, meaning that the type is changing. That raises an error.

bdice · 2020-03-28T03:21:25Z

signac/syncutil.py

@@ -11,14 +11,14 @@

 logger = logging.getLogger('sync')
 logging.addLevelName(LEVEL_MORE, 'MORE')
-logging.MORE = LEVEL_MORE
+logging.MORE = LEVEL_MORE  # type: ignore


Explanation for the ignore: Assigning logging.MORE (and logger.more below) is setting a property that doesn't exist in the original module / class. This kind of monkey-patch raises an error:

signac/syncutil.py:14: error: Module has no attribute "MORE" signac/syncutil.py:21: error: "Logger" has no attribute "more"

bdice · 2020-03-28T03:41:45Z

signac/syncutil.py

+    # The following line must be ignored: https://github.com/python/mypy/issues/708
+    methodmap['same_files'] = methodmap['diff_files'] = phase3  # type: ignore


Explanation: This ignore is due to a long-standing issue in mypy. This is the "third case" described by Guido van Rossum in that issue.

signac/syncutil.py:57: error: Incompatible types in assignment (expression has type "Callable[[dircmp_deep], Any]", target has type "Callable[[], None]")

bdice · 2020-03-28T03:42:43Z

signac/contrib/indexing.py

@@ -159,7 +159,7 @@ class RegexFileCrawler(BaseCrawler):
        MyCrawler.define('.*\/a_(?P<a>\d+)\.txt', 'TextFile')
    """
    "Mapping of compiled regex objects and associated formats."
-    definitions = dict()
+    definitions = dict()  # type: ignore


Explanation: This requests a type annotation for the dict. I added ignores to all deprecated modules where necessary, without questioning it.

bdice · 2020-03-28T03:42:56Z

signac/contrib/filterparse.py

@@ -56,7 +56,7 @@ def _cast(x):
    "Attempt to interpret x with the correct type."
    try:
        if x in CAST_MAPPING_WARNING:
-            print("Did you mean {}?".format(CAST_MAPPING_WARNING[x], file=sys.stderr))
+            print("Did you mean {}?".format(CAST_MAPPING_WARNING[x]), file=sys.stderr)


mypy caught a bug in the print statement! Fixed here.

bdice · 2020-03-28T03:43:05Z

signac/common/host.py

@@ -17,7 +17,7 @@


 SESSION_PASSWORD_HASH_CACHE = SimpleKeyring()
-SESSION_USERNAME_CACHE = dict()
+SESSION_USERNAME_CACHE = dict()  # type: ignore


Explanation: This requests a type annotation for the dict. I added ignores to all deprecated modules where necessary, without questioning it.

bdice · 2020-03-28T03:43:47Z

setup.cfg

@@ -18,6 +18,9 @@ exclude = configobj,passlib,cite.py,conf.py
 match = jsondict.py
 ignore = D105, D107, D203, D213

+[mypy]
+ignore_missing_imports = True


ignore_missing_imports is enabled so that mypy doesn't get mad about missing type annotations in dependencies like deprecation or numpy.

bdice · 2020-03-28T03:45:35Z

I am all in favor of slowly introducing typing to the code base. Is this already checked as part of the CI on this branch?

@csadorf Now it is in CI. This PR is ready for review. After this is approved/merged, I will add type annotations for other files.

csadorf

Two suggestions, otherwise LGTM!

Thank you very much @bdice !

csadorf · 2020-03-28T18:36:07Z

.circleci/config.yml

@@ -31,6 +31,11 @@ jobs:
          command: |
            pydocstyle

+      - run:
+          name: style-check-type-hints


Is this really a style check?

Suggested change

name: style-check-type-hints

name: check-type-hints

I agree -- I was just matching the existing pattern. I renamed the CI job to "pre-checks" so it's all-encompassing, and renamed the individual checks according to their purpose.

signac/syncutil.py

Co-Authored-By: Carl Simon Adorf <[email protected]>

signac/syncutil.py

cbkerr · 2020-04-02T15:54:57Z

Looks good, except should we leave the corrections in a168649 to another branch/PR since it is unrelated to typing, or is it not worth it?

cbkerr · 2020-04-02T15:56:11Z

My mistake.

bdice · 2020-04-02T15:59:12Z

Looks good, except should we leave the corrections in a168649 to another branch/PR since it is unrelated to typing, or is it not worth it?

I decided it wasn't worth putting in its own PR. That's a choice that could be debated but I think it's fine to merge in as-is in the interest of expediency.

cbkerr

I decided it wasn't worth putting in its own PR. That's a choice that could be debated but I think it's fine to merge in as-is in the interest of expediency.

Sounds good!

Add typing to signac.core.json.

7ab5570

bdice added the refactor Code refactoring label Mar 21, 2020

bdice self-assigned this Mar 21, 2020

bdice added 2 commits March 27, 2020 19:42

Merge branch 'master' into feature/typing-signac.core.json

b7d77a5

Run mypy in continuous integration.

d594818

bdice added 2 commits March 27, 2020 19:57

Remove unused FileNotFoundError (likely Python 2 legacy compatibility).

a168649

Ignore missing imports in mypy checks.

af94a5c

bdice added 6 commits March 27, 2020 21:58

Define mypy config.

936c636

Ignore typing in syncutil dircmp_deep.

5b275d3

Fix error in print statement parentheses.

f35944d

Add type ignores.

6fa5657

Remove unused format argument.

fdc5d29

Ignore errors in custom logging level.

9797582

bdice marked this pull request as ready for review March 28, 2020 03:10

bdice requested review from a team as code owners March 28, 2020 03:10

bdice requested review from csadorf and cbkerr and removed request for a team March 28, 2020 03:10

Run mypy on . instead of just signac package.

8a0417a

bdice commented Mar 28, 2020

View reviewed changes

Update changelog.

ba49a25

csadorf approved these changes Mar 28, 2020

View reviewed changes

bdice and others added 2 commits March 29, 2020 14:46

Rename style-check to pre-checks.

43a6387

Update signac/syncutil.py

48aa8be

Co-Authored-By: Carl Simon Adorf <[email protected]>

bdice commented Mar 29, 2020

View reviewed changes

signac/syncutil.py Outdated Show resolved Hide resolved

Update signac/syncutil.py

5f406e2

csadorf approved these changes Mar 30, 2020

View reviewed changes

vyasr mentioned this pull request Mar 31, 2020

Consistently document parameter types glotzerlab/signac-flow#193

Closed

cbkerr closed this Apr 2, 2020

cbkerr reopened this Apr 2, 2020

cbkerr approved these changes Apr 2, 2020

View reviewed changes

bdice merged commit 43a5519 into master Apr 2, 2020

bdice deleted the feature/typing-signac.core.json branch April 2, 2020 16:02

bdice added this to the v1.5.0 milestone Apr 24, 2020

bdice mentioned this pull request Jan 2, 2021

Add type hints glotzerlab/signac-flow#404

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typing to signac.core.json. #313

Add typing to signac.core.json. #313

bdice commented Mar 21, 2020 •

edited

Loading

bdice commented Mar 21, 2020

csadorf commented Mar 23, 2020

bdice commented Mar 24, 2020

csadorf commented Mar 24, 2020

vyasr commented Mar 25, 2020

codecov bot commented Mar 28, 2020 •

edited

Loading

mikemhenry commented Mar 28, 2020

bdice left a comment

bdice Mar 28, 2020

bdice Mar 28, 2020

bdice Mar 28, 2020

bdice Mar 28, 2020

bdice Mar 28, 2020

bdice Mar 28, 2020

bdice Mar 28, 2020

bdice Mar 28, 2020

bdice commented Mar 28, 2020 •

edited

Loading

csadorf left a comment

csadorf Mar 28, 2020

bdice Mar 29, 2020

cbkerr commented Apr 2, 2020

cbkerr commented Apr 2, 2020

bdice commented Apr 2, 2020

cbkerr left a comment

		@@ -22,9 +22,5 @@ class ExportError(Error, RuntimeError):
		pass


		class FileNotFoundError(Error, FileNotFoundError):

		# The following line must be ignored: https://github.com/python/mypy/issues/708
		methodmap['same_files'] = methodmap['diff_files'] = phase3 # type: ignore

Add typing to signac.core.json. #313

Add typing to signac.core.json. #313

Conversation

bdice commented Mar 21, 2020 • edited Loading

Description

Motivation and Context

Types of Changes

Checklist:

bdice commented Mar 21, 2020

csadorf commented Mar 23, 2020

bdice commented Mar 24, 2020

csadorf commented Mar 24, 2020

vyasr commented Mar 25, 2020

codecov bot commented Mar 28, 2020 • edited Loading

Codecov Report

mikemhenry commented Mar 28, 2020

bdice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdice commented Mar 28, 2020 • edited Loading

csadorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbkerr commented Apr 2, 2020

cbkerr commented Apr 2, 2020

bdice commented Apr 2, 2020

cbkerr left a comment

Choose a reason for hiding this comment

bdice commented Mar 21, 2020 •

edited

Loading

codecov bot commented Mar 28, 2020 •

edited

Loading

bdice commented Mar 28, 2020 •

edited

Loading