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

[WIP] Added type hinting #18

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[WIP] Added type hinting #18

wants to merge 2 commits into from

Conversation

6ug
Copy link
Contributor

@6ug 6ug commented Oct 18, 2018

Fixes #11
Totally fine tuned manually! automated tools not handling dynamic execution.

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

Just some preliminary review.

py_ecc/bn128/bn128_curve.py Show resolved Hide resolved
@@ -119,14 +119,14 @@ def zero(cls):


# Utility methods for polynomial math
def deg(p):
def deg(p: Tuple[int, int, int]) -> int:
Copy link
Member

Choose a reason for hiding this comment

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

It may be prudent for us to define a new Point type for this to add an additional level of type safety.

Point = typing.NewType('Point', Tuple[int, int, int])

This prevents passing just any (int, int, int) into these functions, requiring us to be explicit about our point abstraction.

And looking at the code, it appears we'd need both a Point2D and a Point3D to handle the different dimensionalities that are used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pipermerriam can you check this changes e4de2be

py_ecc/secp256k1/secp256k1.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@6ug
Copy link
Contributor Author

6ug commented Oct 18, 2018

PR #19 should fix ^^ above CI failures.

@pipermerriam
Copy link
Member

Looks like ci failures are some small linting errors, and failing due to python 2.7 not supporting type hints.

@carver I'm inclined to drop python 2 support. Can you confirm?

@carver
Copy link
Collaborator

carver commented Oct 19, 2018

I'm inclined to drop python 2 support. Can you confirm?

Yeah, the next release can be v2 👍

@6ug 6ug force-pushed the hint-fixes branch 2 times, most recently from f8b6374 to 3b8e224 Compare October 20, 2018 01:02
@pipermerriam
Copy link
Member

@6ug looks like this needs to be rebased against master

* Added type hints -- removed 12 type annotation
* Adding py_ecc module and mypy changes
* Removed lints errors
* Fixed 3.4 failures
@6ug
Copy link
Contributor Author

6ug commented Oct 27, 2018

@pipermerriam our last typing: Module not found issue was due to - python3.4 does not have inbuilt support for PEP-484 type hinting, we need to externally add typing in this case, d86ca0e will fix that.

@carver
Copy link
Collaborator

carver commented Oct 28, 2018

We should drop py3.4 as well, while we're dropping old python versions. Maybe even py3.5 too.

@pipermerriam
Copy link
Member

ping @6ug are you still working on this?

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

3 participants