Typing python-dotenv: may I submit this to typeshed? #172

qnighy · 2019-03-09T09:50:47Z

In short: I'm asking for permission to write type annotations for python-dotenv and include in the centralized repository called typeshed. In this case, this PR is just for a reference and can be closed. There's another option that I refine the quality of this PR and we start typechecking python-dotenv itself.

I wrote type annotations for python-dotenv which will be useful for python-dotenv users who also use mypy to typecheck their apps.

I'd like to contribute this either:

as an internal annotation in the library (as included in this PR) or
as an external annotation in typeshed.

I do not intend to have this PR merged as-is, rather I've included the changes for reference. In the contribution guide of typeshed, it states:

If you want to submit type stubs for a new library, you need to contact the maintainers of the original library first to let them know and get their permission. Do it by opening an issue on their project's bug tracker. This gives them the opportunity to consider adopting type hints directly in their codebase (which you should prefer to external type stubs). When the project owners agree for you to submit stubs here or you do not receive a reply within one month, open a pull request referencing the issue where you asked for permission.

I was mainly interested in annotating public interfaces, so including this in typeshed is sufficient for me. I also annotated private interfaces but it may be unsatisfactory. However, there's still an option to include the type annotation in python-dotenv and use it for typechecking python-dotenv itself.

Quality of the current type annotation

It passes mypy typecheck in 2.7, 3.5, 3.6, and 3.7 modes.
There are some (non-behavioral) changes in the original code to workaround typechecking problems.
I'm relatively more confident with completeness of public interfaces annotation, than private ones.
Less annotation for IPython support and CLI.

coveralls · 2019-03-09T09:54:33Z

Coverage increased (+0.2%) to 87.797% when pulling 9023fa4 on qnighy:typed into 8c32d75 on theskumar:master.

bbc2

Thank you for this work, it's great!

I like typeshed, so I'm OK with adding stubs there, but I think it's better to put type hints in the code itself. Could you follow the recommendation in https://www.python.org/dev/peps/pep-0561/#packaging-type-information so that users of Python-dotenv don't need typeshed stubs to benefit from type checking? It consists in adding a py.typed marker so that Mypy knows it can use inline type hints from this package.

Your concern about private APIs is legitimate. I think that type hints should not interfere with the development of this library too much and we can refine them incrementally. Therefore, when in doubt, it's definitely OK to omit type hints, ignore some type warnings or use Any types. In particular, it's better than writing complicated type hints that will be hard for other developers to understand, or triggering type warnings when everything is fine (a false positive).

I've done a first pass of review (see inline comments). Other comments:

Since the inline hints should be enough, I think you can remove the stubs (.pyi).
The type hints need to be checked in CI. Could you add a mypy check to the lint tox env? I guess there should be two checks: one with --py2 and the other without.

bbc2 · 2019-03-10T11:52:38Z

src/dotenv/main.py

+                                     ("value", Optional[Text]),
+                                     ("original", Text)])
+else:
+    Binding = namedtuple("Binding", "key value original")


I think you can remove this "else" branch because what you did in the "if" branch looks fine for both typing and execution.

bbc2 · 2019-03-10T12:12:38Z

src/dotenv/main.py

        if isinstance(self.dotenv_path, StringIO):
            yield self.dotenv_path
        elif os.path.isfile(self.dotenv_path):
            with io.open(self.dotenv_path) as stream:
-                yield stream
+                yield stream  # type: ignore


Is this type: ignore necessary? I didn't get any errors if I removed it.

bbc2 · 2019-03-10T12:18:44Z

src/dotenv/main.py

@@ -226,14 +273,17 @@ def unset_key(dotenv_path, key_to_unset, quote_mode="always"):
                dest.write(mapping.original)

    if not removed:
-        warnings.warn("key %s not removed from %s - key doesn't exist." % (key_to_unset, dotenv_path))
+        if not TYPE_CHECKING or sys.version_info >= (3, 0):


I think this obscures the code too much. I would probably just ignore the type warning for the line below. This applies to other uses of warnings.warn.

bbc2 · 2019-03-10T12:19:59Z

src/dotenv/version.py

@@ -1 +1 @@
-__version__ = "0.10.1"
+__version__ = "0.10.1"  # type: str


Mypy should infer the type of __version__ so it shouldn't be necessary to add a type hint for it.

bbc2 · 2019-03-10T12:22:12Z

src/dotenv/main.py

+    if TYPE_CHECKING:
+        dotenv_path = None  # type: Union[Text,_PathLike, _StringIO]
+        _dict = None  # type: Optional[Dict[Text, Text]]
+        verbose = None  # type: bool


I think you can move these type hints to the assignments below in __init__. This removes the need for if TYPE_CHECKING and is probably more accurate since those aren't class variables.

bbc2 · 2019-03-10T12:25:55Z

src/dotenv/main.py

@@ -135,11 +171,13 @@ def set_as_environment_variables(self, override=False):
                if isinstance(k, text_type) or isinstance(v, text_type):
                    k = k.encode('ascii')
                    v = v.encode('ascii')
-            os.environ[k] = v
+            if not TYPE_CHECKING or sys.version_info >= (3, 0):


I think this obscures the code too much. I would probably just ignore the type warning for the line below.

bbc2 · 2019-03-10T12:27:24Z

src/dotenv/main.py

+            if not TYPE_CHECKING or sys.version_info >= (3, 0):
+                warnings.warn("key %s not found in %s." % (key, self.dotenv_path))
+
+        return None # NOTE: PEP8 compliance required by mypy


It's OK to not have a comment here. PEP 8 compliance is implicit in the whole code base and removing this return None shouldn't trigger a lint error (if linting is done in CI).

theskumar · 2019-05-12T10:57:36Z

This is now in v0.10.2, great work @qnighy @bbc2 !! 👍

qnighy added 5 commits March 9, 2019 15:50

Add some type annotations.

68ab22a

Use comment-based annotation.

a7ab3de

Provide fallback for PathLike

134917b

Typecheck on 2.7.

fbfcc4b

Add public stub files.

d715db3

bbc2 self-assigned this Mar 10, 2019

bbc2 requested changes Mar 10, 2019

View reviewed changes

qnighy added 16 commits March 12, 2019 19:37

Add py.typed in accordance with PEP 561.

4fcd0b9

Check mypy types in testenv:lint.

fa6f270

Remove an unnecessary 'type: ignore' directive.

d452173

Use 'type: ignore' instead of complex workarounds.

bc873a3

Remove a type annotation for __version__.

e8114a5

Annotate member types on initialization.

36bac24

Always use NamedTuple.

ff85c44

Fix flake8 errors.

f743880

Remove ./stubs

2981c42

Include src/py.typed in MANIFEST.in.

3665cbb

Add install_requires to dependencies.

15aa870

Also typecheck in 3.4 mode.

3403de6

Move py.typed.

ab17c2e

Text can be reused for text_type.

770d7c9

Ignore TYPE_CHECKING code in coverage.

826195a

Always assert match is not None.

9023fa4

bbc2 approved these changes Mar 30, 2019

View reviewed changes

bbc2 merged commit a25c419 into theskumar:master Mar 30, 2019

vorpal-buildbot mentioned this pull request Jul 27, 2020

Scheduled weekly dependency update for week 30 PennyDreadfulMTG/Penny-Dreadful-Tools#7568

Closed

vorpal-buildbot mentioned this pull request Aug 3, 2020

Scheduled weekly dependency update for week 31 PennyDreadfulMTG/Penny-Dreadful-Tools#7600

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typing python-dotenv: may I submit this to typeshed? #172

Typing python-dotenv: may I submit this to typeshed? #172

qnighy commented Mar 9, 2019

coveralls commented Mar 9, 2019 •

edited

bbc2 left a comment

bbc2 Mar 10, 2019

bbc2 Mar 10, 2019

bbc2 Mar 10, 2019

bbc2 Mar 10, 2019

bbc2 Mar 10, 2019

bbc2 Mar 10, 2019

bbc2 Mar 10, 2019

theskumar commented May 12, 2019

		@@ -1 +1 @@
		__version__ = "0.10.1"
		__version__ = "0.10.1" # type: str

Typing python-dotenv: may I submit this to typeshed? #172

Typing python-dotenv: may I submit this to typeshed? #172

Conversation

qnighy commented Mar 9, 2019

Quality of the current type annotation

coveralls commented Mar 9, 2019 • edited

bbc2 left a comment

Choose a reason for hiding this comment

bbc2 Mar 10, 2019

Choose a reason for hiding this comment

bbc2 Mar 10, 2019

Choose a reason for hiding this comment

bbc2 Mar 10, 2019

Choose a reason for hiding this comment

bbc2 Mar 10, 2019

Choose a reason for hiding this comment

bbc2 Mar 10, 2019

Choose a reason for hiding this comment

bbc2 Mar 10, 2019

Choose a reason for hiding this comment

bbc2 Mar 10, 2019

Choose a reason for hiding this comment

theskumar commented May 12, 2019

coveralls commented Mar 9, 2019 •

edited