[GitHub] [incubator-annotator] Treora opened a new pull request #79: Add tests for text quote selector

[GitBox <gi...@apache.org>]

Treora opened a new pull request #79:
URL: https://github.com/apache/incubator-annotator/pull/79


   See #77. This branch tests description and anchoring of text quote selectors. It does not attempt to exhaust every possible input, nor cover every branch of the code, but uses several example (edge) cases that may likely cause a mistaken implementation to misbehave (‘*pre*gression testing’? :)). In fact I had to make quite some changes in our implementations to make them behave as I would expect.
   
   A few of these expectations are opinionated and desired behaviour may be discussed:
   
   - when trying to describe a range that extends beyond its scope, one could argue describeTextQuote should throw an error; or that it should ignore the parts that fall outside of the scope. I chose the latter for now.
   - where should an empty quote match? Empty quotes may actually be useful when combined with a prefix/suffix, in order to point at a specific point in the text (e.g. to point out where to insert a comma). But an empty quote without prefix/suffix matches at every string position, which may be unwieldy but seems the only coherent behaviour. (matching for a single character can result in an unwieldy amount of matches too, so this should not really be an issue)
   - should prefix and suffix be undefined or the empty string when not needed? I changed it to the latter now in order to follow the spec more closely: *“Each TextQuoteSelector SHOULD have exactly 1 prefix property”* (and same for suffix)
   
   Other changes along the way:
   
   - dropping the `dom-node-iterator` polyfill
   - updating `dom-seek` to v5 (I think we could simplify our code by moving some of the counting logic into that module instead, or build some other abstractions around ‘text node ranges’, but left this for another time)
   - `describeTextQuote` used to run the actual text quote matcher to find out if the selected quote is ambiguous in the given scope. Trying to simplify the code a little, I made it run this search for duplicates itself to avoid the needless conversions between ranges and string indexes; though we may want to return to the original approach some time to allow generalisation to other matchers.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-annotator] Treora edited a comment on pull request #79: Add tests for text quote selector

[GitBox <gi...@apache.org>]

Treora edited a comment on pull request #79:
URL: https://github.com/apache/incubator-annotator/pull/79#issuecomment-635372689


   Note that this branch grows forth from the typescript branch, PR #74, and may best be rebased onto master after that one has been finalised and merged.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-annotator] Treora commented on pull request #79: Add tests for text quote selector

[GitBox <gi...@apache.org>]

Treora commented on pull request #79:
URL: https://github.com/apache/incubator-annotator/pull/79#issuecomment-635372689


   Note that this branch grows forth from the typescript branch, PR #74, and may best be rebased to master after that one has been finalised and merged.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-annotator] Treora merged pull request #79: Add tests for text quote selector

[GitBox <gi...@apache.org>]

Treora merged pull request #79:
URL: https://github.com/apache/incubator-annotator/pull/79


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org