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
merged 6 commits into from Dec 6, 2021

Conversation

12mohaned
Copy link
Contributor

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 .

@tomaarsen
Copy link
Member

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)
else:
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
Member

@tomaarsen tomaarsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Member

@tomaarsen tomaarsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@tomaarsen
Copy link
Member

@12mohaned
#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants