You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@annotator.apache.org by GitBox <gi...@apache.org> on 2020/11/20 23:00:02 UTC

[GitHub] [incubator-annotator] Treora opened a new pull request #98: Implement TextPositionSelector, create Chunking abstraction

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


   Finally.
   
   - Addresses #75 (support TextPositionSelector)
   - Addresses #85 (Chunking abstraction); because having two selectors dealing with text was the right moment to introduce such abstractions. Now the logic of matching&describing a selector is in the selector package, and operates on Chunks (an arbitrary object wrapping a string). Hence the dom package’s main task now is to merely convert a Range into a sequence of chunks (one chunk per text node), and call the abstract implementation with that.
   - Reintroduces Range normalisation (see #51), implemented from scratch.
   - Reimplements and remove need for dom-seek dependency.
   - Updates the demo to show TextPosition, adding a smiley (= two code units) and some elements to make it more challenging.
   
   Things to do still (can wait until after merging):
   - Document everything better; especially the details of the chunking approach.
   - Make tests for the abstract match&describe implementations; test the abstraction layers individually (TextNodeChunker, TextSeeker, CodePointSeeker, normalizeRange); decide how to test the dom-specific match&describe implementations.
   - Possibly also document&publish the abstractions (e.g. CodePointSeeker) as separate components that could be helpful for other purposes than annotation.
   - Modify TextQuoteMatcher to use Seeker, not Chunker (I built further on the reimplementation I made in the chunking branch)
   


----------------------------------------------------------------
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] tilgovi commented on a change in pull request #98: Implement TextPositionSelector, create Chunking abstraction

Posted by GitBox <gi...@apache.org>.
tilgovi commented on a change in pull request #98:
URL: https://github.com/apache/incubator-annotator/pull/98#discussion_r540515237



##########
File path: packages/dom/src/normalize-range.ts
##########
@@ -0,0 +1,156 @@
+/**
+ * @license
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { ownerDocument } from './owner-document';
+
+// TextRange is a Range that guarantees to always have Text nodes as its start
+// and end nodes. To ensure the type remains correct, it also restricts usage
+// of methods that would modify these nodes (note that a user can simply cast
+// the TextRange back to a Range to remove these restrictions).
+export interface TextRange extends Range {
+  readonly startContainer: Text;
+  readonly endContainer: Text;
+  cloneRange(): TextRange;
+
+  // Allow only Text nodes to be passed to these methods.
+  insertNode(node: Text): void;
+  selectNodeContents(node: Text): void;

Review comment:
       What should we do with `selectNode` (as opposed to `selectNodeContents`).




----------------------------------------------------------------
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] tilgovi commented on a change in pull request #98: Implement TextPositionSelector, create Chunking abstraction

Posted by GitBox <gi...@apache.org>.
tilgovi commented on a change in pull request #98:
URL: https://github.com/apache/incubator-annotator/pull/98#discussion_r540513682



##########
File path: .eslintrc.js
##########
@@ -111,6 +112,7 @@ module.exports = {
       plugins: ['@typescript-eslint'],
       rules: {
         '@typescript-eslint/consistent-type-imports': 'error',
+        '@typescript-eslint/no-explicit-any': 'off',

Review comment:
       I agree. We should avoid it where possible but sometimes libraries need to be permissive or ignorant about the types that are passed through APIs.




----------------------------------------------------------------
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 #98: Implement TextPositionSelector, create Chunking abstraction

Posted by GitBox <gi...@apache.org>.
Treora commented on pull request #98:
URL: https://github.com/apache/incubator-annotator/pull/98#issuecomment-750933430


   > I'm not 100 on all the APIs yet, …
   
   Me neither.
   
   > …but I'd like to merge and then make incremental changes from there. Excellent stuff.
   
   Cool with me! I squashed some of the commits to make the path towards this a little more comprehensible.


----------------------------------------------------------------
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] tilgovi commented on a change in pull request #98: Implement TextPositionSelector, create Chunking abstraction

Posted by GitBox <gi...@apache.org>.
tilgovi commented on a change in pull request #98:
URL: https://github.com/apache/incubator-annotator/pull/98#discussion_r540515796



##########
File path: packages/dom/src/normalize-range.ts
##########
@@ -0,0 +1,156 @@
+/**
+ * @license
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { ownerDocument } from './owner-document';
+
+// TextRange is a Range that guarantees to always have Text nodes as its start
+// and end nodes. To ensure the type remains correct, it also restricts usage
+// of methods that would modify these nodes (note that a user can simply cast
+// the TextRange back to a Range to remove these restrictions).
+export interface TextRange extends Range {
+  readonly startContainer: Text;
+  readonly endContainer: Text;
+  cloneRange(): TextRange;
+
+  // Allow only Text nodes to be passed to these methods.
+  insertNode(node: Text): void;
+  selectNodeContents(node: Text): void;

Review comment:
       Oh, I see. It's defined below with `never`.




----------------------------------------------------------------
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 #98: Implement TextPositionSelector, create Chunking abstraction

Posted by GitBox <gi...@apache.org>.
Treora merged pull request #98:
URL: https://github.com/apache/incubator-annotator/pull/98


   


----------------------------------------------------------------
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