Repository: kvesteri/sqlalchemy-utils (language: python)

Issue:
Make explicit it is the python library which is needed
The following exception is not clear as phonenumber can be the sqlalchemy_utils object or the library:

https://github.com/kvesteri/sqlalchemy-utils/blob/500543979ac667fca45dbc1c1b506209b51918e3/sqlalchemy_utils/types/phone_number.py#L174

I suggest to make it explict in the exception and also add to the documentation that it is up to the user to add a dependency on the phonenumber lib. Even maybe suggest to do `pip install phonenumber`  



Discussion / hints:

Fix patch:
diff --git a/setup.cfg b/setup.cfg
index 9d6861e7..4c798761 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -1,4 +1,6 @@
 [flake8]
+max-line-length = 88
+extend-ignore = E203
 exclude =
     .eggs
     .git
diff --git a/sqlalchemy_utils/types/phone_number.py b/sqlalchemy_utils/types/phone_number.py
index 7923cb3a..3ce24b64 100644
--- a/sqlalchemy_utils/types/phone_number.py
+++ b/sqlalchemy_utils/types/phone_number.py
@@ -1,4 +1,11 @@
-import six
+"""
+..  note::
+
+    The `phonenumbers`_ package must be installed to use PhoneNumber types.
+
+..  _phonenumbers: https://github.com/daviddrysdale/python-phonenumbers
+"""
+
 from sqlalchemy import exc, types
 
 from ..exceptions import ImproperlyConfigured
@@ -16,14 +23,15 @@
 
 
 class PhoneNumberParseException(NumberParseException, exc.DontWrapMixin):
-    '''
+    """
     Wraps exceptions from phonenumbers with SQLAlchemy's DontWrapMixin
     so we get more meaningful exceptions on validation failure instead of the
     StatementException
 
     Clients can catch this as either a PhoneNumberParseException or
     NumberParseException from the phonenumbers library.
-    '''
+    """
+
     pass
 
 
@@ -62,9 +70,9 @@ class User(self.Base):
 
         user = User(phone_number=PhoneNumber('0401234567', 'FI'))
 
-        user.phone_number.e164  # u'+358401234567'
-        user.phone_number.international  # u'+358 40 1234567'
-        user.phone_number.national  # u'040 1234567'
+        user.phone_number.e164  # '+358401234567'
+        user.phone_number.international  # '+358 40 1234567'
+        user.phone_number.national  # '040 1234567'
         user.country_code  # 'FI'
 
 
@@ -77,18 +85,17 @@ class User(self.Base):
         should always be True for external callers.
         Can be useful for short codes or toll free
     """
+
     def __init__(self, raw_number, region=None, check_region=True):
         # Bail if phonenumbers is not found.
         if phonenumbers is None:
             raise ImproperlyConfigured(
-                "'phonenumbers' is required to use 'PhoneNumber'"
+                "The 'phonenumbers' package is required to use 'PhoneNumber'"
             )
 
         try:
             self._phone_number = phonenumbers.parse(
-                raw_number,
-                region,
-                _check_region=check_region
+                raw_number, region, _check_region=check_region
             )
         except NumberParseException as e:
             # Wrap exception so SQLAlchemy doesn't swallow it as a
@@ -97,10 +104,7 @@ def __init__(self, raw_number, region=None, check_region=True):
             # Worth noting that if -1 shows up as the error_type
             # it's likely because the API has changed upstream and these
             # bindings need to be updated.
-            raise PhoneNumberParseException(
-                getattr(e, 'error_type', -1),
-                six.text_type(e)
-            )
+            raise PhoneNumberParseException(getattr(e, "error_type", -1), str(e))
 
         super(PhoneNumber, self).__init__(
             country_code=self._phone_number.country_code,
@@ -111,20 +115,17 @@ def __init__(self, raw_number, region=None, check_region=True):
             country_code_source=self._phone_number.country_code_source,
             preferred_domestic_carrier_code=(
                 self._phone_number.preferred_domestic_carrier_code
-            )
+            ),
         )
         self.region = region
         self.national = phonenumbers.format_number(
-            self._phone_number,
-            phonenumbers.PhoneNumberFormat.NATIONAL
+            self._phone_number, phonenumbers.PhoneNumberFormat.NATIONAL
         )
         self.international = phonenumbers.format_number(
-            self._phone_number,
-            phonenumbers.PhoneNumberFormat.INTERNATIONAL
+            self._phone_number, phonenumbers.PhoneNumberFormat.INTERNATIONAL
         )
         self.e164 = phonenumbers.format_number(
-            self._phone_number,
-            phonenumbers.PhoneNumberFormat.E164
+            self._phone_number, phonenumbers.PhoneNumberFormat.E164
         )
 
     def __composite_values__(self):
@@ -158,20 +159,21 @@ class User(self.Base):
 
         user = User(phone_number='+358401234567')
 
-        user.phone_number.e164  # u'+358401234567'
-        user.phone_number.international  # u'+358 40 1234567'
-        user.phone_number.national  # u'040 1234567'
+        user.phone_number.e164  # '+358401234567'
+        user.phone_number.international  # '+358 40 1234567'
+        user.phone_number.national  # '040 1234567'
     """
-    STORE_FORMAT = 'e164'
+
+    STORE_FORMAT = "e164"
     impl = types.Unicode(20)
     python_type = PhoneNumber
     cache_ok = True
 
-    def __init__(self, region='US', max_length=20, *args, **kwargs):
+    def __init__(self, region="US", max_length=20, *args, **kwargs):
         # Bail if phonenumbers is not found.
         if phonenumbers is None:
             raise ImproperlyConfigured(
-                "'phonenumbers' is required to use 'PhoneNumberType'"
+                "The 'phonenumbers' package is required to use 'PhoneNumberType'"
             )
 
         super(PhoneNumberType, self).__init__(*args, **kwargs)
@@ -183,8 +185,8 @@ def process_bind_param(self, value, dialect):
             if not isinstance(value, PhoneNumber):
                 value = PhoneNumber(value, region=self.region)
 
-            if self.STORE_FORMAT == 'e164' and value.extension:
-                return '%s;ext=%s' % (value.e164, value.extension)
+            if self.STORE_FORMAT == "e164" and value.extension:
+                return "%s;ext=%s" % (value.e164, value.extension)
 
             return getattr(value, self.STORE_FORMAT)
 
diff --git a/tox.ini b/tox.ini
index 9112c10c..74f79d3a 100644
--- a/tox.ini
+++ b/tox.ini
@@ -38,8 +38,8 @@ commands =
     isort --verbose --check-only .
 skip_install = True
 deps =
-    flake8>=3.7.9
-    isort>=4.3.21
+    flake8
+    isort
 
 [pytest]
 filterwarnings =


Test patch:
diff --git a/tests/types/test_phonenumber.py b/tests/types/test_phonenumber.py
index 37fcd8a3..02729794 100644
--- a/tests/types/test_phonenumber.py
+++ b/tests/types/test_phonenumber.py
@@ -1,5 +1,4 @@
 import pytest
-import six
 import sqlalchemy as sa
 
 from sqlalchemy_utils import (  # noqa
@@ -10,23 +9,24 @@
 )
 
 VALID_PHONE_NUMBERS = (
-    '040 1234567',
-    '+358 401234567',
-    '09 2501234',
-    '+358 92501234',
-    '0800 939393',
-    '09 4243 0456',
-    '0600 900 500'
+    "040 1234567",
+    "+358 401234567",
+    "09 2501234",
+    "+358 92501234",
+    "0800 939393",
+    "09 4243 0456",
+    "0600 900 500",
 )
 
 
 @pytest.fixture
 def User(Base):
     class User(Base):
-        __tablename__ = 'user'
+        __tablename__ = "user"
         id = sa.Column(sa.Integer, autoincrement=True, primary_key=True)
         name = sa.Column(sa.Unicode(255))
         phone_number = sa.Column(PhoneNumberType())
+
     return User
 
 
@@ -37,57 +37,47 @@ def init_models(User):
 
 @pytest.fixture
 def phone_number():
-    return PhoneNumber(
-        '040 1234567',
-        'FI'
-    )
+    return PhoneNumber("040 1234567", "FI")
 
 
 @pytest.fixture
 def user(session, User, phone_number):
     user = User()
-    user.name = u'Someone'
+    user.name = "Someone"
     user.phone_number = phone_number
     session.add(user)
     session.commit()
     return user
 
 
-@pytest.mark.skipif('types.phone_number.phonenumbers is None')
+@pytest.mark.skipif("types.phone_number.phonenumbers is None")
 class TestPhoneNumber:
-    @pytest.mark.parametrize('raw_number', VALID_PHONE_NUMBERS)
+    @pytest.mark.parametrize("raw_number", VALID_PHONE_NUMBERS)
     def test_valid_phone_numbers(self, raw_number):
-        number = PhoneNumber(raw_number, 'FI')
+        number = PhoneNumber(raw_number, "FI")
         assert number.is_valid_number()
 
-    @pytest.mark.parametrize('raw_number', ('abc', '+040 1234567'))
+    @pytest.mark.parametrize("raw_number", ("abc", "+040 1234567"))
     def test_invalid_phone_numbers__constructor_fails(self, raw_number):
         with pytest.raises(PhoneNumberParseException):
-            PhoneNumber(raw_number, 'FI')
+            PhoneNumber(raw_number, "FI")
 
-    @pytest.mark.parametrize('raw_number', ('0111234567', '358'))
+    @pytest.mark.parametrize("raw_number", ("0111234567", "358"))
     def test_invalid_phone_numbers__is_valid_number(self, raw_number):
-        number = PhoneNumber(raw_number, 'FI')
+        number = PhoneNumber(raw_number, "FI")
         assert not number.is_valid_number()
 
-    def test_invalid_phone_numbers_throw_dont_wrap_exception(
-        self,
-        session,
-        User
-    ):
+    def test_invalid_phone_numbers_throw_dont_wrap_exception(self, session, User):
         with pytest.raises(PhoneNumberParseException):
             session.execute(
-                User.__table__.insert().values(
-                    name=u'Someone',
-                    phone_number=u'abc'
-                )
+                User.__table__.insert().values(name="Someone", phone_number="abc")
             )
 
     def test_phone_number_attributes(self):
-        number = PhoneNumber('+358401234567')
-        assert number.e164 == u'+358401234567'
-        assert number.international == u'+358 40 1234567'
-        assert number.national == u'040 1234567'
+        number = PhoneNumber("+358401234567")
+        assert number.e164 == "+358401234567"
+        assert number.international == "+358 40 1234567"
+        assert number.national == "040 1234567"
 
     def test_phone_number_attributes_for_short_code(self):
         """
@@ -96,56 +86,45 @@ def test_phone_number_attributes_for_short_code(self):
         raise exception
         :return:
         """
-        number = PhoneNumber('72404', check_region=False)
-        assert number.e164 == u'+072404'
-        assert number.international == u'72404'
-        assert number.national == u'72404'
+        number = PhoneNumber("72404", check_region=False)
+        assert number.e164 == "+072404"
+        assert number.international == "72404"
+        assert number.national == "72404"
 
     def test_phone_number_str_repr(self):
-        number = PhoneNumber('+358401234567')
-        if six.PY2:
-            assert unicode(number) == number.national  # noqa
-            assert str(number) == number.national.encode('utf-8')
-        else:
-            assert str(number) == number.national
+        number = PhoneNumber("+358401234567")
+        assert str(number) == number.national
 
     def test_phone_number_hash(self):
-        number1 = PhoneNumber('+821023456789')
-        number2 = PhoneNumber('+82 10-2345-6789')
+        number1 = PhoneNumber("+821023456789")
+        number2 = PhoneNumber("+82 10-2345-6789")
         assert hash(number1) == hash(number2)
         assert hash(number1) == hash(number1.e164)
         assert {number1} == {number2}
 
 
-@pytest.mark.skipif('types.phone_number.phonenumbers is None')
+@pytest.mark.skipif("types.phone_number.phonenumbers is None")
 class TestPhoneNumberType:
-    def test_query_returns_phone_number_object(
-        self,
-        session,
-        User,
-        user,
-        phone_number
-    ):
+    def test_query_returns_phone_number_object(self, session, User, user, phone_number):
         queried_user = session.query(User).first()
         assert queried_user.phone_number == phone_number
 
     def test_phone_number_is_stored_as_string(self, session, user):
         result = session.execute(
-            'SELECT phone_number FROM "user" WHERE id=:param',
-            {'param': user.id}
+            'SELECT phone_number FROM "user" WHERE id=:param', {"param": user.id}
         )
-        assert result.first()[0] == u'+358401234567'
+        assert result.first()[0] == "+358401234567"
 
     def test_phone_number_with_extension(self, session, User):
-        user = User(phone_number='555-555-5555 Ext. 555')
+        user = User(phone_number="555-555-5555 Ext. 555")
 
         session.add(user)
         session.commit()
         session.refresh(user)
-        assert user.phone_number.extension == '555'
+        assert user.phone_number.extension == "555"
 
     def test_empty_phone_number_is_equiv_to_none(self, session, User):
-        user = User(phone_number='')
+        user = User(phone_number="")
 
         session.add(user)
         session.commit()
@@ -155,24 +134,23 @@ def test_empty_phone_number_is_equiv_to_none(self, session, User):
     def test_uses_phonenumber_class_as_python_type(self):
         assert PhoneNumberType().python_type is PhoneNumber
 
-    @pytest.mark.usefixtures('user')
+    @pytest.mark.usefixtures("user")
     def test_phone_number_is_none(self, session, User):
         phone_number = None
         user = User()
-        user.name = u'Someone'
+        user.name = "Someone"
         user.phone_number = phone_number
         session.add(user)
         session.commit()
         queried_user = session.query(User)[1]
         assert queried_user.phone_number is None
         result = session.execute(
-            'SELECT phone_number FROM "user" WHERE id=:param',
-            {'param': user.id}
+            'SELECT phone_number FROM "user" WHERE id=:param', {"param": user.id}
         )
         assert result.first()[0] is None
 
     def test_scalar_attributes_get_coerced_to_objects(self, User):
-        user = User(phone_number='050111222')
+        user = User(phone_number="050111222")
 
         assert isinstance(user.phone_number, PhoneNumber)
 
@@ -182,38 +160,30 @@ def test_compilation(self, User, session):
         session.execute(query)
 
 
-@pytest.mark.skipif('types.phone_number.phonenumbers is None')
+@pytest.mark.skipif("types.phone_number.phonenumbers is None")
 class TestPhoneNumberComposite:
     @pytest.fixture
     def User(self, Base):
         class User(Base):
-            __tablename__ = 'user'
+            __tablename__ = "user"
             id = sa.Column(sa.Integer, autoincrement=True, primary_key=True)
             name = sa.Column(sa.String(255))
             _phone_number = sa.Column(sa.String(255))
             country = sa.Column(sa.String(255))
-            phone_number = sa.orm.composite(
-                PhoneNumber,
-                _phone_number,
-                country
-            )
+            phone_number = sa.orm.composite(PhoneNumber, _phone_number, country)
+
         return User
 
     @pytest.fixture
     def user(self, session, User):
         user = User()
-        user.name = u'Someone'
-        user.phone_number = PhoneNumber('+35840111222', 'FI')
+        user.name = "Someone"
+        user.phone_number = PhoneNumber("+35840111222", "FI")
         session.add(user)
         session.commit()
         return user
 
-    def test_query_returns_phone_number_object(
-        self,
-        session,
-        User,
-        user
-    ):
+    def test_query_returns_phone_number_object(self, session, User, user):
         queried_user = session.query(User).first()
-        assert queried_user.phone_number.national == '040 111222'
-        assert queried_user.phone_number.region == 'FI'
+        assert queried_user.phone_number.national == "040 111222"
+        assert queried_user.phone_number.region == "FI"