Address review comments around add_extension method

- Fix typo in the docs (s/buidlder/builder/)
- Remove default from the method declaration and docs
- Replace ValueError with NotImpelementedError for unsupported X.509
  extensions
- Add TODO comment as requested by Alex
- Fix test to pass critical=False since it no longer is a default value

3 files changed, 8 insertions, 6 deletions

diff --git a/docs/x509.rst b/docs/x509.rst
index 84b3b8b4..c4c441e7 100644
--- a/docs/x509.rst
+++ b/docs/x509.rst
@@ -490,7 +490,7 @@ X.509 CSR (Certificate Signing Request) Builder Object
         >>> builder = builder.subject_name(x509.Name([
         ...     x509.NameAttribute(x509.OID_COMMON_NAME, u'cryptography.io'),
         ... ]))
-        >>> buidlder = builder.add_extension(
+        >>> builder = builder.add_extension(
         ...     x509.BasicConstraints(ca=False, path_length=None), critical=True,
         ... )
         >>> request = builder.sign(
@@ -506,7 +506,7 @@ X.509 CSR (Certificate Signing Request) Builder Object
         :returns: A new
             :class:`~cryptography.x509.CertificateSigningRequestBuilder`.
 
-    .. method:: add_extension(extension, critical=False)
+    .. method:: add_extension(extension, critical)
 
         :param extension: The :class:`~cryptography.x509.Extension` to add to
             the request.
diff --git a/src/cryptography/x509.py b/src/cryptography/x509.py
index f59ea78a..21e18ddd 100644
--- a/src/cryptography/x509.py
+++ b/src/cryptography/x509.py
@@ -1462,14 +1462,15 @@ class CertificateSigningRequestBuilder(object):
             raise ValueError('The subject name may only be set once.')
         return CertificateSigningRequestBuilder(name, self._extensions)
 
-    def add_extension(self, extension, critical=False):
+    def add_extension(self, extension, critical):
         """
         Adds an X.509 extension to the certificate request.
         """
         if isinstance(extension, BasicConstraints):
             extension = Extension(OID_BASIC_CONSTRAINTS, critical, extension)
         else:
-            raise ValueError('Unsupported X.509 extension.')
+            raise NotImplementedError('Unsupported X.509 extension.')
+        # TODO: This is quadratic in the number of extensions
         for e in self._extensions:
             if e.oid == extension.oid:
                 raise ValueError('This extension has already been set.')
diff --git a/tests/test_x509.py b/tests/test_x509.py
index 441d634b..78def5f8 100644
--- a/tests/test_x509.py
+++ b/tests/test_x509.py
@@ -784,9 +784,10 @@ class TestCertificateSigningRequestBuilder(object):
                 x509.NameAttribute(x509.OID_COMMON_NAME, u'cryptography.io'),
             ])
         )
-        with pytest.raises(ValueError):
+        with pytest.raises(NotImplementedError):
             builder.add_extension(
-                x509.AuthorityKeyIdentifier('keyid', None, None)
+                x509.AuthorityKeyIdentifier('keyid', None, None),
+                critical=False,
             )