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:
us...@infra.apache.org
