Type hinting #28

ckp95 · 2020-12-06T14:38:17Z

I've added type hints to this package, so that static type checkers like mypy will work.

I thought that it would be a simple change, but it turned out to require reorganizing a lot of the package structure. I couldn't figure out a way to avoid doing this unfortunately.

I also altered the travis.yml to fix an unrelated build failure.

On Python<3.5, the typing module is an external dependency, so we can't import from jdcal in setup.py because the dependency hasn't been installed yet. So the version must be moved to a separate file and then both setup.py and jdcal.py both import from it.

This was causing build failures on ppc64le, see: https://travis-ci.community/t/permission-issue-while-building-wheels- for-various-python-packages/7822/14

In PEP 561, it is stated that we need to include the file py.typed in order for type checkers to be able to use the type hints: https://www.python.org/dev/peps/pep-0561/#packaging-type-information It also says that module-only distributions are not supported and so the code needs to be refactored into a package-based distribution. Therefore I moved jdcal.py into its own package directory. This then necessitated importing its functions in the __init__.py so that the API won't change (i.e. keep jdcal.is_leap rather than jdcal.jdcal.is_leap). But mypy --strict isn't happy with this unless you also explicitly re- export everything via __all__ or do from X import Y as Y on everything. I don't understand why this extra layer of redundancy is required but I can't find a way around it: https://mypy.readthedocs.io/en/latest/config_file.html#confval-implicit _reexport

phn · 2021-02-18T04:08:14Z

Hello @ckp95

That is a lot of work you have put in! I appreciate that.

Do we really need this? I am trying to keep the code base as simple as possible, to ensure
it works for anyone no matter what Python version there are using, and to keep
dependencies as few as possible. I am able to do this, since the underlying code
is simple and uses nothing more that arithmetic and trigonometric functions.

So my opinion is that we don't merge this PR.

I would like to hear your opinion before I close this.

Thanks again for your contribution.

ckp95 · 2021-02-18T12:33:02Z

There are no problems wrt Python versions -- I made sure to use the comment version of type hints so that it works on everything back to Python 2.7, you can see this in the CI tests.

It introduces no further external dependencies for users of the package. All the typing stuff comes from the standard library. There is the addition of mypy for the build/release process, but that will not affect users.

I think this is an improvement to the package, and will benefit projects that rely on jdcal as a dependency since IDEs can provide the correct type information, and static type checkers will behave properly.

jdufresne · 2021-02-18T17:26:04Z

FWIW, I'm in favor of seeing type hints added. Using the comment syntax ensures the code can remain compatible with legacy EOL Pythons.

phn · 2021-02-19T03:49:44Z

@ckp95 @jdufresne

You have convinced me that this is a good thing!

@ckp95

Can you get the changes made in master since you cloned the repository
into this PR? This includes changes such as deleting travis,
setting up Github workflow, adding CONTRIBUTORS.txt and so on. This
should cause this PR to be tested on Github actions instead of travis
(at-least I think it will since actions are configured to run on PRs).
One Python question: looking at setup.py it seems that the typing backport
module will get installed for all Python versions. So, when importing typing,
in jdcal or other libraries the user has installed, in Python versions that
already have typing in stdlib, which version of typing will be imported? And
could this cause any issues?

ckp95 · 2021-02-19T12:32:47Z

I will look at this on the weekend.
Ah I was actually wrong earlier about the lack of introduced dependencies, I forgot that the typing module isn't native to Python<3.5 and has to be installed. However I don't think this is a huge issue: first, it is maintained by the PSF so it could still be considered as part of the standard library but one step removed, and second, it is a dependency of many other packages so it is unlikely that users of old Python versions would not already have it installed. As for conflicts on newer versions: this is not a problem, as the docs for the typing module state:

NOTE: in Python 3.5 and later, the typing module lives in the stdlib, and installing this package has NO EFFECT, because stdlib takes higher precedence than the installation directory.

jdufresne · 2021-02-19T13:10:05Z

If you want, you should be able to avoid the typing dependency by using the if False: trick:

if False:
     from typing import Tuple

mypy understands this and will recognize the Tuple type but at runtime, the import is skipped. This is a common pattern when adding types to Python 2 code bases.

jdufresne

Overall, really nice. Thanks for contributing this.

jdufresne · 2021-02-19T13:11:57Z

MANIFEST.in

@@ -1,3 +1,4 @@
 include README.rst
 include LICENSE.txt
-include *jdcal.py
+include src/jdcal/*.py


This should be included automatically by setuptools. We only need to mention the test files:

Suggested change

include src/jdcal/*.py

include tests/*.py

jdufresne · 2021-02-19T13:14:02Z

setup.py

@@ -35,5 +35,9 @@
        'Programming Language :: Python :: Implementation :: CPython',
        'Programming Language :: Python :: Implementation :: PyPy',
    ],
-    py_modules=["jdcal"]
+    # py_modules=["jdcal"],


Don't need to keep this.

Suggested change

# py_modules=["jdcal"],

jdufresne · 2021-02-19T13:24:20Z

setup.py

@@ -35,5 +35,9 @@
        'Programming Language :: Python :: Implementation :: CPython',
        'Programming Language :: Python :: Implementation :: PyPy',
    ],
-    py_modules=["jdcal"]
+    # py_modules=["jdcal"],
+    packages=find_packages("src"),


As the package has no subpackages, find_packages is overkill, IMO:

Suggested change

packages=find_packages("src"),

packages=["jdcal"],

Won't some of these suggestions change since we have decided to go with the latest recommendation
of using setup.cfg to specify such information and keeping setup.py to the minimum?

When adjusting setup.cfg there will be a choice of using find_packages or listing the packages. IMO, listing them is better as the layout is relatively simple. So I think the comment still applies, but just to the new declarative syntax.

jdufresne · 2021-02-19T13:26:24Z

setup.py

-    py_modules=["jdcal"]
+    # py_modules=["jdcal"],
+    packages=find_packages("src"),
+    package_dir={"": "src"},


You should also mention the package_data here:

Suggested change

package_dir={"": "src"},

package_data={"jdcal": ["py.typed"]},

package_dir={"": "src"},

jdufresne · 2021-02-19T13:29:44Z

src/jdcal/__init__.py

@@ -0,0 +1,14 @@
+#!/usr/bin/env python


This file is a package and has no external side effects, it should not be executable as is.

Suggested change

#!/usr/bin/env python

jdufresne · 2021-02-19T13:30:00Z

src/jdcal/version.py

@@ -0,0 +1,4 @@
+#!/usr/bin/env python


Suggested change

#!/usr/bin/env python

jdufresne · 2021-02-19T13:30:43Z

tox.ini

+
+[testenv:mypy]
+deps = mypy
+commands = mypy --strict src/jdcal/jdcal.py


We should type check the entire package, not just one file.

Suggested change

commands = mypy --strict src/jdcal/jdcal.py

commands = mypy --strict src/

jdufresne · 2021-02-19T13:32:15Z

tox.ini


 [testenv:docstyle]
 deps = pydocstyle
-commands = pydocstyle jdcal.py test_jdcal.py
+commands = pydocstyle src/jdcal/jdcal.py 


Same, run on full package.

jdufresne · 2021-02-19T13:32:26Z

tox.ini

@@ -13,8 +13,12 @@ deps =

 [testenv:codestyle]
 deps = pycodestyle
-commands = pycodestyle --ignore=E722,E501 jdcal.py test_jdcal.py
+commands = pycodestyle --ignore=E722,E501 src/jdcal/jdcal.py tests/test_jdcal.py


run on entire directory.

jdufresne · 2021-02-19T13:33:46Z

src/jdcal/__init__.py

+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+
+# see https://mypy.readthedocs.io/en/latest/config_file.html#confval-implicit_reexport


IMO, it is more conventional to express exports using __all__

__all__ = ["__version__", "MJD_0", ...]

avalentino · 2022-02-26T09:36:38Z

@ckp95 this would be a very interesting addition.
Do you have time to re-base on the current master and address the remaining comments?

ckp95 added 11 commits December 5, 2020 20:30

add type hints

a8f8fc3

install typing in ppc64le

3df42fe

move version number to __init__.py

72e00b9

add __init__.py to MANIFEST.in

95d6ab2

add typing to setup.py, change version import

c78e5f7

tidy up tox.ini

7d586ad

revert change to travis.yml

5728cd3

remove before_install section of travis.yml

d562dd3

This was causing build failures on ppc64le, see: https://travis-ci.community/t/permission-issue-while-building-wheels- for-various-python-packages/7822/14

add mypy to travis build process

1009b9c

jdufresne reviewed Feb 19, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type hinting #28

Type hinting #28

ckp95 commented Dec 6, 2020

phn commented Feb 18, 2021

ckp95 commented Feb 18, 2021

jdufresne commented Feb 18, 2021

phn commented Feb 19, 2021

ckp95 commented Feb 19, 2021 •

edited

Loading

jdufresne commented Feb 19, 2021

jdufresne left a comment

jdufresne Feb 19, 2021

jdufresne Feb 19, 2021

jdufresne Feb 19, 2021

phn Feb 20, 2021

jdufresne Feb 20, 2021

jdufresne Feb 19, 2021

jdufresne Feb 19, 2021

jdufresne Feb 19, 2021

jdufresne Feb 19, 2021

jdufresne Feb 19, 2021

jdufresne Feb 19, 2021

jdufresne Feb 19, 2021

avalentino commented Feb 26, 2022

	package_dir={"": "src"},
	package_data={"jdcal": ["py.typed"]},
	package_dir={"": "src"},

	commands = mypy --strict src/jdcal/jdcal.py
	commands = mypy --strict src/

Type hinting #28

Are you sure you want to change the base?

Type hinting #28

Conversation

ckp95 commented Dec 6, 2020

phn commented Feb 18, 2021

ckp95 commented Feb 18, 2021

jdufresne commented Feb 18, 2021

phn commented Feb 19, 2021

ckp95 commented Feb 19, 2021 • edited Loading

jdufresne commented Feb 19, 2021

jdufresne 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

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

avalentino commented Feb 26, 2022

ckp95 commented Feb 19, 2021 •

edited

Loading