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 improved static type checking in setup.py #221

Merged
merged 4 commits into from
Apr 23, 2021
Merged

Add improved static type checking in setup.py #221

merged 4 commits into from
Apr 23, 2021

Conversation

kibablu
Copy link
Contributor

@kibablu kibablu commented Apr 21, 2021

Add static type checking on setup.py with Any as the path-like object is either str or bytes object representing a path, or an object implementing the os path.

fixes #201

Signed-off-by: Omar Mohamed <kibablu16@gmail.com>
fixes #201

Signed-off-by: Omar Mohamed <kibablu16@gmail.com>
setup.py Outdated
Comment on lines 29 to 30
def read(fname: Any) -> Any:
return open(os.path.join(os.path.dirname(__file__), fname)).read()
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, declaring Any doesn't help us catch invalid values. For example, this function would throw an exception if passed an integer, but mypy wouldn't warn us ahead of time since everything matches Any.

What do you think these types should be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of str but we also have bytes objects representing a path, is it possible to combine str and bytes

Copy link
Contributor

Choose a reason for hiding this comment

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

fixes #201

Signed-off-by: Omar Mohamed <kibablu16@gmail.com>
@callahad
Copy link
Contributor

I'm surprised this is failing.

Let me test if I can get this to blow up locally, and if so, whether adding from __future__ import annotations fixes it or if there's something else we need to do.

@callahad
Copy link
Contributor

Okay, yep. This is failing for a very obscure reason. Python 3.7 and 3.8 don't understand the [str] part of PathLike[str], and so they throw an error.

We can fix that by opting into new behaviors for type annotations. Making the following change should get the tests to pass:

diff --git a/setup.py b/setup.py
index 88a79f5..2e91d7a 100755
--- a/setup.py
+++ b/setup.py
@@ -16,9 +16,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from typing import Union
-from os import PathLike
+from __future__ import annotations
+
 import os.path
+from os import PathLike
+from typing import Union
 
 from setuptools import find_packages, setup
 

...and since we're using the __future__ import, we can also use the new shorthand syntax for Unions:

diff --git a/setup.py b/setup.py
index 2e91d7a..303c791 100755
--- a/setup.py
+++ b/setup.py
@@ -20,7 +20,6 @@ from __future__ import annotations
 
 import os.path
 from os import PathLike
-from typing import Union
 
 from setuptools import find_packages, setup
 
@@ -29,7 +28,7 @@ from setuptools import find_packages, setup
 # Used for the long_description.  It's nice, because now 1) we have a top level
 # README file and 2) it's easier to type in the README file than to put a raw
 # string in below ...
-def read(fname: Union[str, PathLike[str]]) -> str:
+def read(fname: str | PathLike[str]) -> str:
     return open(os.path.join(os.path.dirname(__file__), fname)).read()
 
 

fixes #201

Signed-off-by: Omar Mohamed <kibablu16@gmail.com>
@kibablu
Copy link
Contributor Author

kibablu commented Apr 23, 2021

Thank you @callahad 😃 🎉 🎉

Copy link
Contributor

@callahad callahad left a comment

Choose a reason for hiding this comment

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

And the tests pass! Yay! Thank you so much

@callahad callahad merged commit 2c574ea into matrix-org:master Apr 23, 2021
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.

None yet

2 participants