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

Hotfix/bugs on wordnet app #3008

Merged
merged 2 commits into from
May 25, 2022

Conversation

StephanHasler
Copy link
Contributor

This pull request resolves two bugs in the wordnet_app (#3002 and #3003).

@arademaker
Copy link

I am assuming the use of HTML in the code like

template = """
is because NLTK is avoiding have to depend on web frameworks like Flask right?

@StephanHasler
Copy link
Contributor Author

I guess the use of a web framework like Flask could lead to less and simpler code. I would prefer that.
The question is, if it makes sense to port the wordnet_app. It seems no one uses it. The wordnet online search is more informative. I used the wordnet_app only once when the online search was not available.

@arademaker
Copy link

arademaker commented May 23, 2022

Hi @StephanHasler, I am curious, what 'wordnet online search' are you using?

  1. The Princeton web interface for PWN 3.1 at https://wordnet.princeton.edu?
  2. The http://en-word.net from the Open English WordNet (fork from Princeton WN 3.0/3.1) from @jmccrae ?
  3. The multilingual http://compling.hss.ntu.edu.sg/omw/ from @fcbond ?
  4. The Polish/English wordnets interface at http://plwordnet.pwr.wroc.pl/wordnet/?

I also have my own web interface for the Portuguese Wordnet and Princeton English Wordnet 3.0 at http://openwordnet-pt.org.

@StephanHasler
Copy link
Contributor Author

I use the princeton Web interface. I was not aware of the others.
I would not have fixed the wordnet_app, if I knew the alternatives, e.g. Open English Wordnet.
Thanks for telling me!

@purificant
Copy link
Member

LGTM

I was able to reproduce both of #3002 and #3003 and the change set in this PR prevents the issues from occurring in my testing.

@tomaarsen tomaarsen merged commit 6f18391 into nltk:develop May 25, 2022
@tomaarsen
Copy link
Member

I trust your judgement on this @purificant. Beyond that, these changes seem sensible when inspected.
I appreciate your work on this @StephanHasler! Especially the latter bug seems nasty to chase down.

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

Successfully merging this pull request may close these issues.

None yet

4 participants