-
Notifications
You must be signed in to change notification settings - Fork 12
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
prevent unknown or misspelled annotators from being added to the annotator-array #49
prevent unknown or misspelled annotators from being added to the annotator-array #49
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just couple of comments, It’s a good improvement as it helps understanding what’s going on for this case. Thanks for contributing!!
src/pipeline.js
Outdated
if ( | ||
Object.prototype.hasOwnProperty.call(ANNOTATORS_BY_KEY, annotatorKey) | ||
) { | ||
acc.push(ANNOTATORS_BY_KEY[annotatorKey]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d expect a pure function being passed as reducer. Instead of push you can return acc.concat([newelem]); (sorry the syntax, i’m With my phone) 🙃
src/pipeline.js
Outdated
) { | ||
acc.push(ANNOTATORS_BY_KEY[annotatorKey]); | ||
} else { | ||
console.log(`Annotator "${annotatorKey}" doesn't exist or is not supported. Removing it from the pipeline.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t remember if I used console before, maybe throwing an error instead? It should not just warn you in that case, it should stop the execution as is a misconfiguration based on this wrapper limitations.
Thanks for the quick feedback. Sounds absolutely reasonable! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…dded to the annotator-array (#49)
Hi,
thanks for creating this wrapper! I like it a lot. :)
When I recently tried to use it with the "openie" annotator the program threw an unhandled exception. It seems like Annotators that are valid for CoreNLP but non-existend in ANNOTATORS_BY_KEY will add an undefined element to the annotator-list, causing problems down the line.
To reproduce, just add openie to the list of annotators.
I added a check to prevent that and added logging for the user.
This is my first pull request, so please let me know if you spot any mistakes I've made.