-
Notifications
You must be signed in to change notification settings - Fork 101
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
Show how letters and words can be veiled #716
Conversation
There were tiny changes made to some existing processors classes, but only for convenience, and the webapp output was beautified. Otherwise, the functionality lies in Veil.scala, which can be scrapped or sit there harmlessly if need be. |
This feels very complicated. I think I'd like an example before I'm convinced... |
There are two examples (veilText, veilDocument) at the bottom of Veil.scala. Would those suffice? |
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.
Nice!
Just to be safe, we should ask @myedibleenso for his feedback as well.
This PR has been updated with the changes to master. In review, it includes
@myedibleenso, I'd like to get this merged as soon as you can look at it. Unless something is wrong with the changes to Document and Sentence, this code will probably just sit around until someone needs it. In a PR is not a good place to sit, though. |
val document = processor.mkDocument(text) | ||
val veiledWords = Seq((0, Range.inclusive(document.sentences(0).raw.indexOf("("), document.sentences(0).raw.indexOf(")")))) | ||
val veiledDocument = new VeiledDocument(document, veiledWords) | ||
val annotatedDocument = veiledDocument.annotate(processor) |
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.
Would be good to add a type hint here to make it clear that annotatedDocument
is a Document
.
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.
Yikes! Sorry, @kwalcock . I somehow lost track of this PR from ages ago. Thank you for the reminder. I think this is going to be very useful for a bunch of tasks.
Good point. It's even better than a hint: it's enforced. |
I think this is ready to be merged. Thanks @kwalcock ! |
No description provided.