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

Anafora Reader #25

Closed
wants to merge 13 commits into from
Closed

Anafora Reader #25

wants to merge 13 commits into from

Conversation

EgoLaparra
Copy link
Contributor

No description provided.

Copy link
Collaborator

@bethard bethard left a comment

Choose a reason for hiding this comment

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

Looking much closer. Anafora.scala and TypesTest.scala still appear to be unnecessarily attached to the commit. Also, GitHub is reporting the pull request as failing tests. Take a look and see if you can see why.

@@ -42,6 +43,26 @@ class Entity(xml: Elem) extends Annotation(xml) {
def text(implicit data: Data): String = spans.map{
case (start, end) => data.text.substring(start, end)
}.mkString("...")
private def recursiveSpan(entity: Entity, start: Int, end: Int)(implicit data:Data): (Int, Int) = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These changes don't seem to belong in this pull request; as far as I can tell, AnaforaReader doesn't use any of these methods.


class AnaforaReader(dct: SimpleInterval)(implicit data: Data) {

val DCT = dct
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just declare it as AnaforaReader(val DCT: SimpleInterval) if you want a publicly accessible DCT.

@@ -1042,5 +1042,12 @@ class TypesTest extends FunSuite with TypesSuite {
NthFromStartRIs(Year(1997), 1, RepeatingUnit(ChronoUnit.MONTHS), 9)
=== SimpleIntervals((1 to 9).map(m => SimpleInterval.of(1997, m))))
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem relevant to the pull request.

@EgoLaparra EgoLaparra closed this Aug 4, 2017
@EgoLaparra EgoLaparra deleted the areader branch August 4, 2017 16:24
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.

2 participants