Fix traitlets warnings when running tests

[kinow]

Running tests locally got warnings such as

(venv) kinow@ranma:~/Development/python/workspace/ldapauthenticator$ pytest
============================================================================= test session starts =============================================================================
platform linux -- Python 3.8.3, pytest-5.4.3, py-1.9.0, pluggy-0.13.1
rootdir: /home/kinow/Development/python/workspace/ldapauthenticator, inifile: pytest.ini
plugins: asyncio-0.14.0, requests-mock-1.8.0, cov-2.10.0
collected 7 items                                                                                                                                                             

ldapauthenticator/tests/test_ldapauthenticator.py .......                                                                                                               [100%]

============================================================================== warnings summary ===============================================================================
ldapauthenticator/ldapauthenticator.py:72
  /home/kinow/Development/python/workspace/ldapauthenticator/ldapauthenticator/ldapauthenticator.py:72: DeprecationWarning: metadata {'default': None} was set from the constructor. With traitlets 4.1, metadata should be set using the .tag() method, e.g., Int().tag(key1='value1', key2='value2')
    allowed_groups = List(

-- Docs: https://docs.pytest.org/en/latest/warnings.html
======================================================================== 7 passed, 1 warning in 1.27s =========================================================================

Fixed using the .tag. Tests passing locally, no more warnings

(venv) kinow@ranma:~/Development/python/workspace/ldapauthenticator$ pytest
============================================================================= test session starts =============================================================================
platform linux -- Python 3.8.3, pytest-5.4.3, py-1.9.0, pluggy-0.13.1
rootdir: /home/kinow/Development/python/workspace/ldapauthenticator, inifile: pytest.ini
plugins: asyncio-0.14.0, requests-mock-1.8.0, cov-2.10.0
collected 7 items                                                                                                                                                             

ldapauthenticator/tests/test_ldapauthenticator.py .......                                                                                                               [100%]

============================================================================== 7 passed in 1.26s ==============================================================================

[manics]

The warning suggests default is being treated as metadata on the property which implies it probably doesn't do anything useful.

Looking at https://traitlets.readthedocs.io/en/stable/trait_types.html is sounds like what was originally intended is default_value, however that may now change the behaviour of the existing properties so perhaps we should just delete default completely? @minrk ?

[kinow]

Thanks @manics . I had forgotten about this PR. Found and fixed a typo, and also rebased. Happy to update or close depending on @minrk 's answer 👍

[minrk]

Yup, these want to be default_value.

Background:

• traits have a dict called 'metadata' which can be used for filtering/selecting traits (e.g. for widget state sync)
• this used to be initialized using all unrecognized keyword arguments passed to constructors. This was deprecated in favor of an explicit .tag() method because it allowed typos exactly like the one here to go unnoticed!
• The deprecated behavior is not disabled due to traitlets' very conservative "try not to break anything" policy, hence the warning here, hinting at a real bug.

So this PR as it is preserves the current behavior avoiding the warning, but the current behavior is not the intended behavior.

Aside: It is a little weird and not a pattern I particularly endorse to use allow_none=True on Unicode traits when a Falsy empty string will do. Personally, I try to only use that when an empty string has a particular and valid meaning that needs to distinguish from "no value", which is rare. The fact that we haven't had any bug reports about this suggests that we are not relying on the None behavior (because the default value is in fact an empty string due to the typo here).

[kinow]

So this PR as it is preserves the current behavior avoiding the warning, but the current behavior is not the intended behavior.

@minrk edited the commit to use default_value=None, and confirmed the warning that happens on master does not happen after the change.

Aside: It is a little weird and not a pattern I particularly endorse to use allow_none=True on Unicode traits when a Falsy empty string will do. Personally, I try to only use that when an empty string has a particular and valid meaning that needs to distinguish from "no value", which is rare. The fact that we haven't had any bug reports about this suggests that we are not relying on the None behavior (because the default value is in fact an empty string due to the typo here).

With the following diff, tests pass with no errors too.

diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py
index 4e3a809..a7de2c7 100644
--- a/ldapauthenticator/ldapauthenticator.py
+++ b/ldapauthenticator/ldapauthenticator.py
@@ -72,7 +72,7 @@ class LDAPAuthenticator(Authenticator):
     allowed_groups = List(
         config=True,
         allow_none=True,
-        default_value=None,
+        default_value=[],
         help="""
         List of LDAP group DNs that users could be members of to be granted access.
 
@@ -118,7 +118,7 @@ class LDAPAuthenticator(Authenticator):
 
     user_search_base = Unicode(
         config=True,
-        default_value=None,
+        default_value='',
         allow_none=True,
         help="""
         Base for looking up user accounts in the directory, if `lookup_dn` is set to True.
@@ -144,7 +144,7 @@ class LDAPAuthenticator(Authenticator):
 
     user_attribute = Unicode(
         config=True,
-        default_value=None,
+        default_value='',
         allow_none=True,
         help="""
         Attribute containing user's name, if `lookup_dn` is set to True.

But not too sure if there are no side effects. I had a look at a few uses of user_attribute, and found nothing that would prevent us changing it to empty strings/list. Or should we do it later in a separate PR?

Thanks!

[minrk]

Let's land this change and not worry about it too much. In the future, I think leaving default_value undefined is usually best, at least for new traits.

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

[kinow]

Thanks @minrk ! And also @manics for reviewing it 👍