Adding trial type annotations to functions with correct return types. #202

mitchellkennedy · 2021-08-04T10:05:19Z

I added a trial set of type annotations relating to #200 for the following functions:
parse
_extract_possible_number
_is_viable_phone_number
_normalize
normalize_digits_only
normalize_diallable_chars_only
convert_alpha_characters_in_number

This has been tested on 3.9, 3.8, 3.7, 3.6, 3.5, and 2.7. thus far.

daviddrysdale

I've got some initial questions that I suspect just expose my ignorance of Python type annotations…

daviddrysdale · 2021-08-04T12:17:09Z

python/phonenumbers/phonenumberutil.py

@@ -552,6 +552,7 @@ def _copy_number_format(other):


 def _extract_possible_number(number):
+    # type: (str) -> None


I'd naively expect this to be (str) -> str rather than None – maybe I need to learn more about Python type annotations…

I believe you are correct. I was attempting to follow the convention laid out in examples provided in PEP 484 here. The code they include is as follows:

def embezzle(self, account, funds=1000000, *fake_receipts): # type: (str, int, *str) -> None """Embezzle funds from account using fake receipts.""" <code goes here>

I was assuming they were using None for some sort of compatibility reason, but digging further into the documents, this is meant to specify the return type. I will correct this and submit another PR.

daviddrysdale · 2021-08-04T12:17:38Z

python/phonenumbers/phonenumberutil.py

@@ -588,6 +589,7 @@ def _extract_possible_number(number):


 def _is_viable_phone_number(number):
+    # type: (str) -> None


Likewise, (str) -> bool?

As previously noted, you are correct.

daviddrysdale · 2021-08-04T12:20:05Z

python/phonenumbers/phonenumberutil.py

@@ -2772,6 +2778,7 @@ def _set_italian_leading_zeros_for_phone_number(national_number, numobj):

 def parse(number, region=None, keep_raw_input=False,
          numobj=None, _check_region=True):
+    # type: (str, str, bool, **Any, bool) -> None


Again, without knowing much about Python type annotations I'd expect the 4th parameter to have a type like Optional[PhoneNumber] – does that not work?

(BTW, the codebase has a general convention that a parameter called numobj is supposed to be a PhoneNumber object)

I have just tested this under Python 2 and 3, and it does appear to work. Pardon my confusion on the backwards compatible aspects. I will amend the next PR with this as well.

Cool, sounds good – thanks for doing this.

By the way, how are you checking that the type annotations work? I'm thinking that I could/should add some type checking to the continuous integration GitHub actions.

No problem, happy to contribute!

To be specific, what I am implementing is type commenting rather than annotating. Both were introduced in Python 3, but annotating has not been backported to Python 2. Commenting is the suggested alternative to annotating if backwards compatibility needs to be maintained under the heading "Suggested syntax for Python 2.7 and straddling code" in PEP 484.
I've been validating by ensuring the examples I wrote run correctly and also that my IDE (PyCharm) correctly parses and code-completes the functions for which the type hints have been added.
Thinking about checking, I am sure there would be some way to implement that, but I'm not certain at this moment how. Since I have been digging into the documentation, I will give this some thought and raise a feature request if I can come up with something that works once this pull request is complete.

I've got a hack to do mypy checking in #203; it will be interesting to see how it combines with this PR…

I don't yet understand the failures in https://github.com/daviddrysdale/python-phonenumbers/runs/3363632239?check_suite_focus=true so I think I've got some more reading to do.

daviddrysdale · 2021-08-18T09:21:15Z

python/phonenumbers/phonenumberutil.py

@@ -2772,6 +2778,7 @@ def _set_italian_leading_zeros_for_phone_number(national_number, numobj):

 def parse(number, region=None, keep_raw_input=False,
          numobj=None, _check_region=True):
+    # type: (str, str, bool, PhoneNumber, bool) -> Optional[PhoneNumber]


Can this ever return None (i.e. should the return type just be PhoneNumber)?

This line specifically is stating that it should always return PhoneNumber. None can be used just as any other type. It is possibly worth noting that for obvious reasons keeping hinting correct and accurate is the preferred method of operation, but any violation will not result in an exception.

Doesn't the -> Optional[PhoneNumber] at the end imply that this function can return None as a normal return value (because Optional[T] is the same as Union[T, None])?

In other words I'm suspecting that the last part should be -> PhoneNumber.

I completely misunderstood what you were asking, sorry. Earlier in this thread, you had used Optional on that line as an example. I was under the impression that you were suggesting that should be the production code for a reason I had not looked hard enough to understand.
I have now removed that and pushed that change.

Apologies for confusing things, I should have included my guesses at the full type signature.

… which I think should be:

# type: (str, str, bool, Optional[PhoneNumber], bool) -> PhoneNumber

(i.e. the function always returns a PhoneNumber object not None, but the 4th parameter can be either None or PhoneNumber)

WDYT?

daviddrysdale · 2021-08-18T09:22:32Z

python/phonenumbers/phonenumberutil.py

@@ -2772,6 +2778,7 @@ def _set_italian_leading_zeros_for_phone_number(national_number, numobj):

 def parse(number, region=None, keep_raw_input=False,
          numobj=None, _check_region=True):
+    # type: (str, str, bool, **Any, bool) -> None


I've got a hack to do mypy checking in #203; it will be interesting to see how it combines with this PR…

daviddrysdale · 2021-09-02T08:53:59Z

@mitchellkennedy thanks for getting the ball rolling with this – your initial type annotations look to match those in #207, so that's been helpful as a cross-check.

(Closing because I think this is now subsumed by #207)

Adding trial type annotations to functions

8817c8a

daviddrysdale reviewed Aug 4, 2021

View reviewed changes

Added trial type comments to functions

fbaf3bd

mitchellkennedy changed the title ~~Adding trial type annotations to functions~~ Adding trial type annotations to functions with correct return types. Aug 5, 2021

daviddrysdale reviewed Aug 18, 2021

View reviewed changes

corrected return type

586f298

AA-Turner mentioned this pull request Sep 1, 2021

Add API type definitions through stub files #207

Merged

daviddrysdale closed this Sep 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding trial type annotations to functions with correct return types. #202

Adding trial type annotations to functions with correct return types. #202

mitchellkennedy commented Aug 4, 2021

daviddrysdale left a comment

daviddrysdale Aug 4, 2021

mitchellkennedy Aug 5, 2021 •

edited

Loading

daviddrysdale Aug 4, 2021

mitchellkennedy Aug 5, 2021

daviddrysdale Aug 4, 2021

mitchellkennedy Aug 5, 2021

daviddrysdale Aug 5, 2021

mitchellkennedy Aug 5, 2021

daviddrysdale Aug 18, 2021

daviddrysdale Aug 18, 2021

daviddrysdale Aug 18, 2021

mitchellkennedy Aug 18, 2021

daviddrysdale Aug 18, 2021

mitchellkennedy Aug 18, 2021

daviddrysdale Aug 18, 2021

daviddrysdale Aug 18, 2021

daviddrysdale commented Sep 2, 2021

		@@ -552,6 +552,7 @@ def _copy_number_format(other):


		def _extract_possible_number(number):
		# type: (str) -> None

		@@ -588,6 +589,7 @@ def _extract_possible_number(number):


		def _is_viable_phone_number(number):
		# type: (str) -> None

Adding trial type annotations to functions with correct return types. #202

Adding trial type annotations to functions with correct return types. #202

Conversation

mitchellkennedy commented Aug 4, 2021

daviddrysdale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchellkennedy Aug 5, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddrysdale commented Sep 2, 2021

mitchellkennedy Aug 5, 2021 •

edited

Loading