Repository: nltk/nltk
Pull request: Deprecate 'return_str' parameter in NLTKWordTokenizer and TreebankWordTokenizer
Resolves #2879 

As per the discussion on #2879, I have deprecated the use of the 'return_str' parameter in the `tokenize` method of both `NLTKWordTokenizer` and `TreebankWordTokenizer`.

I have currently done this in a way that raises a `DeprecationWarning` if a non-default value is passed to the parameter. 

Problem statement:
Deprecate `return_str` parameter in `NLTKWordTokenizer` and `TreebankWordTokenizer`
Hello!

I'd like to discuss a potential enhancements of `NLTKWordTokenizer` and `TreebankWordTokenizer`. For those unaware, the former is the tokenizer that is most frequently used, and is used in the `word_tokenize` function. It's also based on the latter class: `TreebankWordTokenizer`.

An example usage as can be found in the documentation:

```python
>>> from nltk.tokenize import NLTKWordTokenizer
>>> s = '''Good muffins cost $3.88 (roughly 3,36 euros)\nin New York.  Please buy me\ntwo of them.\nThanks.'''
>>> NLTKWordTokenizer().tokenize(s)
['Good', 'muffins', 'cost', '$', '3.88', '(', 'roughly', '3,36',
'euros', ')', 'in', 'New', 'York.', 'Please', 'buy', 'me', 'two',
'of', 'them.', 'Thanks', '.']
>>> NLTKWordTokenizer().tokenize(s, convert_parentheses=True)
['Good', 'muffins', 'cost', '$', '3.88', '-LRB-', 'roughly', '3,36',
'euros', '-RRB-', 'in', 'New', 'York.', 'Please', 'buy', 'me', 'two',
'of', 'them.', 'Thanks', '.']
>>> NLTKWordTokenizer().tokenize(s, return_str=True)
' Good muffins cost  $ 3.88  ( roughly 3,36 euros ) \nin New York.  Please buy me\ntwo of them.\nThanks  .  '
```

### The enhancement
As you can see from the example, if `return_str` is True, then the `tokenize` method returns a space-separated string. However, the number of spaces is very inconsistent. Perhaps we would be better off stripping spaces on the ends, and replacing all sequences of multiple spaces with just one space.

E.g. 
```python
>>> NLTKWordTokenizer().tokenize(s, return_str=True)
'Good muffins cost $ 3.88 ( roughly 3,36 euros ) \nin New York. Please buy me\ntwo of them.\nThanks .'
``` 
instead of 
```python
>>> NLTKWordTokenizer().tokenize(s, return_str=True)
' Good muffins cost  $ 3.88  ( roughly 3,36 euros ) \nin New York.  Please buy me\ntwo of them.\nThanks  .  '
```

I figured I would create an issue for this to find out if others agree with this idea, before I put in the time to make this change for no reason. So, I'd like to hear your thoughts.

- Tom Aarsen

Hints from issue discussion:
Could we also consider deprecating this usage... I wonder why we did this!
Yeah, sounds good. I can't really imagine a use case for this parameter to begin with.
Hi, I would like to pick this up as my first issue if possible. Does NLTK have a standard way of deprecating parameters already? I have seen the `@Deprecated` decorator but I am not sure how I could use it to deprecate the parameter only.
I don't believe so, no. The decorator are only for deprecating classes, methods and functions, iirc.

There's at least three options here:
1. Raising a warning when `return_str` is True, but having the functionality the same. In the future, we would remove this functionality, and for now we just warn users. This would be like a slow deprecation.
2. Raising a warning when `return_str` is True, and updating the functionality to ignore the `return_str` parameter. This would be like a fast deprecation.
3. Simply deleting all references of `return_str`. This would break implementations that currently use `return_str` without giving feedback. This would be like a sudden deprecation.

I really don't see the use of `return_str`, so I don't have issues with going for option 2, so we don't need to revisit this in a few months time. Perhaps other people disagree with me though.
As can be seen in the PR above (#2883), I have opted for option 2 in agreement with @tomaarsen's comment but am happy to rework if another solution is preferred.

Commit message: Deprecated 'return_str' parameter in NLTKWordTokenizer and TreebankWordTokenizer

Patch to review:
diff --git a/AUTHORS.md b/AUTHORS.md
index 5b7546d43c..04592e11cc 100644
--- a/AUTHORS.md
+++ b/AUTHORS.md
@@ -285,6 +285,7 @@
 - Saibo Geng <https://github.com/Saibo-creator>
 - Ahmet Yildirim <https://github.com/RnDevelover>
 - Yuta Nakamura <https://github.com/yutanakamura-tky>
+- Adam Hawley <https://github.com/adamjhawley>
 
 ## Others whose work we've taken and included in NLTK, but who didn't directly contribute it:
 
diff --git a/nltk/tokenize/destructive.py b/nltk/tokenize/destructive.py
index 32eb64fd8f..80e30d3e21 100644
--- a/nltk/tokenize/destructive.py
+++ b/nltk/tokenize/destructive.py
@@ -8,6 +8,7 @@
 
 
 import re
+import warnings
 
 from nltk.tokenize.api import TokenizerI
 from nltk.tokenize.util import align_tokens
@@ -113,6 +114,15 @@ class NLTKWordTokenizer(TokenizerI):
     CONTRACTIONS3 = list(map(re.compile, _contractions.CONTRACTIONS3))
 
     def tokenize(self, text, convert_parentheses=False, return_str=False):
+
+        if return_str is not False:
+            warnings.warn(
+                "Parameter 'return_str' has been deprecated and should no "
+                "longer be used.",
+                category=DeprecationWarning,
+                stacklevel=2,
+            )
+
         for regexp, substitution in self.STARTING_QUOTES:
             text = regexp.sub(substitution, text)
 
@@ -147,7 +157,7 @@ def tokenize(self, text, convert_parentheses=False, return_str=False):
         # for regexp in self._contractions.CONTRACTIONS4:
         #     text = regexp.sub(r' \1 \2 \3 ', text)
 
-        return text if return_str else text.split()
+        return text.split()
 
     def span_tokenize(self, text):
         r"""
diff --git a/nltk/tokenize/treebank.py b/nltk/tokenize/treebank.py
index 51c2020130..571bea77d2 100644
--- a/nltk/tokenize/treebank.py
+++ b/nltk/tokenize/treebank.py
@@ -18,6 +18,7 @@
 """
 
 import re
+import warnings
 
 from nltk.tokenize.api import TokenizerI
 from nltk.tokenize.destructive import MacIntyreContractions
@@ -99,6 +100,15 @@ class TreebankWordTokenizer(TokenizerI):
     CONTRACTIONS3 = list(map(re.compile, _contractions.CONTRACTIONS3))
 
     def tokenize(self, text, convert_parentheses=False, return_str=False):
+
+        if return_str is not False:
+            warnings.warn(
+                "Parameter 'return_str' has been deprecated and should no "
+                "longer be used.",
+                category=DeprecationWarning,
+                stacklevel=2,
+            )
+
         for regexp, substitution in self.STARTING_QUOTES:
             text = regexp.sub(substitution, text)
 
@@ -133,7 +143,7 @@ def tokenize(self, text, convert_parentheses=False, return_str=False):
         # for regexp in self._contractions.CONTRACTIONS4:
         #     text = regexp.sub(r' \1 \2 \3 ', text)
 
-        return text if return_str else text.split()
+        return text.split()
 
     def span_tokenize(self, text):
         r"""


Code review comments:
diff_hunk: @@ -113,6 +114,15 @@ class NLTKWordTokenizer(TokenizerI):
     CONTRACTIONS3 = list(map(re.compile, _contractions.CONTRACTIONS3))
 
     def tokenize(self, text, convert_parentheses=False, return_str=False):
+
+        if return_str is not False:
path: nltk/tokenize/destructive.py
text: @user2:
```suggestion
        if return_str:
```
Well spotted @user1! This is also more in line with the old behaviour of `text if return_str else text.split()`.