Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deprecate: deprecate get and set node methods #2900

merged 6 commits into from Dec 6, 2021


Copy link

@12mohaned 12mohaned commented Dec 2, 2021

This pull request deprecates _get_node() and _set_node() methods, it was similar to this PR, but I forgot to amend the required changes instead, I accidentally removed the methods. Sorry for the inconvenience @stevenbird .

Copy link

@tomaarsen tomaarsen commented Dec 2, 2021

NLTK has implemented a useful decorator for cases like these. I recommend that we use it. See an example below:

@deprecated("Use Nk, Nik or Nck instead")
def N(self, k=None, i=None, c=None):
"""Implements the "n-notation" used in Artstein and Poesio (2007)"""
if k is not None and i is None and c is None:
ret = self.Nk(k)
elif k is not None and i is not None and c is None:
ret = self.Nik(i, k)
elif k is not None and c is not None and i is None:
ret = self.Nck(c, k)
raise ValueError(
f"You must pass either i or c, not both! (k={k!r},i={i!r},c={c!r})"
log.debug("Count on N[%s,%s,%s]: %d", k, i, c, ret)
return ret

i.e. simply adding @deprecated("Use set_label() instead") before both of the methods. The body can become pass or kept how it was. See the code for deprecated here.

Copy link

@tomaarsen tomaarsen left a comment

This is definitely preferable, however the _get_node() is deprecated section of the deprecation message is unnecessary, as @deprecated already adds this. Right now, the output is:

>>> from nltk.tree import Tree
>>> t = Tree.fromstring('(S (NP (D the) (N dog)) (VP (V chased) (NP (D the) (N cat))))')
>>> t._get_node()  
<stdin>:1: DeprecationWarning: 
  Function _get_node() has been deprecated.  _get_node() is
  deprecated, use label() instead

Copy link

@tomaarsen tomaarsen left a comment

Looks good to me

Copy link

@tomaarsen tomaarsen commented Dec 6, 2021

#2903 has fixed a broken test case which snuck into this PR. I've merged develop into this PR to resolve this broken test.

@tomaarsen tomaarsen merged commit ba989e5 into nltk:develop Dec 6, 2021
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants