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/08/23 04:20:39 UTC
[GitHub] [incubator-annotator] tilgovi opened a new pull request #87: Eliminate extraneous interfaces
tilgovi opened a new pull request #87:
URL: https://github.com/apache/incubator-annotator/pull/87
Make several changes to eliminate unnecessary type definitions.
Remove the centrally-exported selector interfaces from the selector
package and migrate them to where they are used. The selector package
need not commit to the existence of any particular set of selectors. In
the future, the apache-annotator package may consolidate the existing
selector definitions and export them under a unified namespace.
Remove the matcher interface in favor of being explicit and verbose
about function types.
Simplify the type signature for makeRefinable. The selector is defined
as an interface, thus callers are free to pass anything that extends it,
so there is no need to parametrize its type. Use the same type
definition for the input and output of the selector creator.
Define the DOM match functions in terms of Range and place the burden on
the caller to pass a Range. Remove the unused scope functions. This
change may hurt the developer experience, but that will require some
experience and feedback.
Remove the "type" field from all selector definitions, since the match
functions do not use this information. Applications may decide to
include type information in selectors, but it is not necessary to
dictate the specific way to achieve this. In the future, utility
functions in the selector package may dictate this by providing a typed
selector creator that delegates to implementations based on a type
field.
----------------------------------------------------------------
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 pull request #87: Eliminate extraneous interfaces
Posted by GitBox <gi...@apache.org>.
tilgovi commented on pull request #87:
URL: https://github.com/apache/incubator-annotator/pull/87#issuecomment-681312139
I'll take another look at `makeRefinable` and test out how it behaves when called from TypeScript on different inputs.
I also think it may be okay to add back the `Matcher` parametrized type, but might avoid aliasing it with filled in parameters to avoid the relative path lint issues _for now_. I do want to solve that code organization question, though, and I think we could eventually add `DomMatcher` back in.
----------------------------------------------------------------
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 pull request #87: Eliminate extraneous interfaces
Posted by GitBox <gi...@apache.org>.
tilgovi commented on pull request #87:
URL: https://github.com/apache/incubator-annotator/pull/87#issuecomment-690362276
Closing this. We pulled the DOM scope change out into #88 and we're investigating a different pattern for composing the higher level match function creators to be re-introduced in another PR.
----------------------------------------------------------------
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 #87: Eliminate extraneous interfaces
Posted by GitBox <gi...@apache.org>.
Treora commented on pull request #87:
URL: https://github.com/apache/incubator-annotator/pull/87#issuecomment-679141635
> Make several changes to eliminate unnecessary type definitions.
Are we talking about “unnecessary” in a pedagogical or technical sense? :) (my view has been that types are primarily there to help readers create an mental overview of the concepts that we deal with in the code base; implementation needs come second)
> Remove the centrally-exported selector interfaces from the selector
> package and migrate them to where they are used. The selector package
> need not commit to the existence of any particular set of selectors. In
> the future, the apache-annotator package may consolidate the existing
> selector definitions and export them under a unified namespace.
:question: My thinking was rather that the type definitions would be based on the WA specification, rather than on the needs of our implementation. The type definitions could be considered a product by itself, in theory even usable without using any of our implementations. But we could also decide that this is not a goal of ours.
> Remove the matcher interface in favor of being explicit and verbose about function types.
:+1: If we use the same signature throughout, and as part of our user-facing API, give the signature a name seems helpful; on the other hand, if that hides its definition it might also be obscuring things. With our current small API either way seems fine to me, so we could just remove them now and see if we feel a need to reintroduce such names at some point.
> Simplify the type signature for makeRefinable. The selector is defined
> as an interface, thus callers are free to pass anything that extends it,
> so there is no need to parametrize its type. …
:-1: But callers are not free to pass any type of `Selector`. When assigning `f = makeRefinable(createTextQuoteSelectorMatcher)`, `f`’s parameter must still be a `TextQuoteSelector`, not just any `Selector`.
In a quick test, after this change even `makeRefinable(createTextQuoteSelectorMatcher)` seems to be invalid because of mismatching types, so this needs more scrutiny anyway. (perhaps we could make some integration tests, and/or turn the demo into typescript, to discover such errors more easily)
> … Use the same type definition for the input and output of the selector creator.
:+1:
> Define the DOM match functions in terms of Range and place the burden on
> the caller to pass a Range. Remove the unused scope functions. This
> change may hurt the developer experience, but that will require some
> experience and feedback.
:+1: Hurting developer experience sounds undesirable; but this change makes the code and API simpler, which may actually help developers. For ease of use, we could consider providing a simple helper to turn a nodes into Range:
```
export function nodeToRange(node: Node): Range {
const range = (node.ownerDocument ?? node as Document).createRange();
range.selectNodeContents(node);
return range;
}
```
> Remove the "type" field from all selector definitions, since the match
> functions do not use this information. Applications may decide to
> include type information in selectors, but it is not necessary to
> dictate the specific way to achieve this. In the future, utility
> functions in the selector package may dictate this by providing a typed
> selector creator that delegates to implementations based on a type
> field.
:question: Indeed technically we do not need the `type` field, but (in case we take a spec-first focus) that seems an implementation detail. On the other hand, it’s also a bit silly to require adding the `type` everywhere (e.g. in our test cases) just for spec conformance. As a compromise, we could also consider defining the types according to spec, and then creating supertypes using `Pick<…, …>` or `Omit<…, 'type'>` in our implementation.
I’m fine with merging the proposed changes (after fixing the refinement issue) and then reconsidering some aspects later once we better know what we’d want. I suppose the whole distinction between selector and dom package, and what exactly they export, will only make sense once we have tools for non-dom environments.
----------------------------------------------------------------
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 pull request #87: Eliminate extraneous interfaces
Posted by GitBox <gi...@apache.org>.
tilgovi commented on pull request #87:
URL: https://github.com/apache/incubator-annotator/pull/87#issuecomment-681311670
> My thinking was rather that the type definitions would be based on the WA specification, rather than on the needs of our implementation. The type definitions could be considered a product by itself, in theory even usable without using any of our implementations. But we could also decide that this is not a goal of ours.
I share the goal of defining selectors that correspond to the WA specification. I would prefer that the individual selector packages define the selectors they use, though, rather than trying to define them all in a single selectors package.
> If we use the same signature throughout, and as part of our user-facing API, give the signature a name seems helpful; on the other hand, if that hides its definition it might also be obscuring things. With our current small API either way seems fine to me, so we could just remove them now and see if we feel a need to reintroduce such names at some point.
That's my thought, too. I wasn't sure if it made things easier or obscured things yet. I also wasn't sure how I wanted to resolve the relative path linting yet, either. This change allows us to punt these questions down the road a little bit.
> But callers are not free to pass any type of Selector.
I had a fear about this and I'll investigate. I thought that the type parameter on the scope would be sufficient to restrict the input match function creator, and that using an generic Selector interface here would still allow arbitrary other selectors to be passed in. Since Selector has only an optional `refinedBy` attribute, any selector should be fine, so long as it does not redefine `refinedBy` to be anything else. I do want to capture the notion of iterative refinement, but maybe parametrization of the Selector interface would allow us to define `makeRefinable` such that the output match function creator accepts and produces a sub-type of Selector that conforms to the original input type of the provided `createMatcher` function.
> Hurting developer experience sounds undesirable; but this change makes the code and API simpler, which may actually help developers. For ease of use, we could consider providing a simple helper to turn a nodes into Range:
That is my thinking, too. It gives us a good reason to expose the functions I deleted from the scope module. We may even want to formalize the idea of creating different types of scopes for the different packages. For example, we might want the DOM scope to be something that has attributes for the owner document, the range, and maybe an iteration protocol for nodes.
> Indeed technically we do not need the type field, but (in case we take a spec-first focus) that seems an implementation detail.
The specification is written in terms of JSON-LD, where individual attributes are absolute URIs to linked data properties. There's a canonical short-hand of attribute names provided by the JSON-LD context, but I was thinking that it's not necessary to require that the selectors have a `type` field with any particular value. If the caller knows that a selector is a text quote selector, then we can assume its attribute refer to the attributes of a text quote selector, but the type is implicit.
I think it will make sense to define helpers, like a typed matcher creator that we have in the demo. That would be a good addition to the selector package. That is a good place to be opinionated about the type field, while letting the user have control of its values.
``` typescript
createTypedMatcherCreator({
TextQuoteSelector: createTextQuoteSelectorMatcher
});
```
----------------------------------------------------------------
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 closed pull request #87: Eliminate extraneous interfaces
Posted by GitBox <gi...@apache.org>.
tilgovi closed pull request #87:
URL: https://github.com/apache/incubator-annotator/pull/87
----------------------------------------------------------------
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