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

Misspelled function parameter for MosesTokenizer: agressive_dash_splits #1955

Closed
goodmami opened this issue Feb 14, 2018 · 4 comments
Closed

Comments

@goodmami
Copy link
Contributor

goodmami commented Feb 14, 2018

Just a minor issue. The agressive_dash_splits is misspelled. It should be aggressive_dash_splits. Or maybe use hyphen instead of dash to be consistent with both the AGGRESSIVE_HYPHEN_SPLIT class member and with tokenizer.perl.

http://www.nltk.org/api/nltk.tokenize.html#nltk.tokenize.moses.MosesTokenizer.tokenize

Also this functionality does not appear to be tested.

@goodmami goodmami changed the title Misspelled function parameter for MosesTokenizer: agressive_hyphen_splits Misspelled function parameter for MosesTokenizer: agressive_dash_splits Feb 14, 2018
@goodmami
Copy link
Contributor Author

Thanks @somnathrakshit for the quick PR. Note that altering the parameter name breaks the API, so it might be better to first provide it as an option with a DeprecationWarning when the old parameter name is used, then it can be fully removed at the next major version. Maybe a regular NLTK dev can comment on the procedure here, as I didn't see it mentioned explicitly in the developer guidelines or CONTRIBUTING.md doc. @alvations, are there any guidelines or precedents for changing function/parameter names?

@alvations
Copy link
Contributor

alvations commented Feb 15, 2018

@goodmami @somnathrakshit no worries about breaking API in this case. Most people would be more stymied by the typo argument instead of the correct one =)

Regarding deprecation and breaking user space, in this case it's our fault and it's easier for users to update to new NLTK version.

But in other cases, esp. when it comes to more major changes that's not just typo, we'll can use warnings like what we did with deprecating Stanford tools https://github.com/nltk/nltk/blob/develop/nltk/tag/stanford.py#L51

@somnathrakshit
Copy link
Contributor

Thanks @alvations for letting us know. As a beginner in open source, nltk has been nice to tinker with. Are you taking part in GSoC 2018?

@alvations
Copy link
Contributor

Resolved in #1956

@somnathrakshit Thanks for the contribution! Unfortunately, we're not taking part in GSoC 2018. Perhaps another year when we have more volunteers =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants