Treora commented on pull request #88: URL: https://github.com/apache/incubator-annotator/pull/88#issuecomment-689082728
> I'll approve, but recommend renaming `util.ts` to just `owner-document.ts` or `scope.ts`. As it is not specific to scopes and now also used for other ranges, `owner-document.ts` seems better. > I felt that it was a couple lines of simple destructing and one nullish coalescing assignment. In context, it might often be useful to destructure other properties of the range at the same time. I'm fine to have a helper function, but it didn't feel necessary or very clarifying. Ok. I think we have different priorities; for me, even if it is just two or three lines of code, the fact that the *intention* of the code is somewhat obscured makes it an undesirable distraction for a reader. I suppose it deserves a comment line to explain the need for the type assertion, so I’ll add that while at it. Moreover I don’t see the usefulness of destructuring in these cases, as it results in variable names that hide their context (“whose ‘commonAncestorContainer’ are we talking about, that of scope or of the other range?”). It may be clear by now that I write code primarily for people, not computers. :) I like folders, but I’m okay with flatter packages and/or more packages. As long as it is easy to find out which things belong together. ---------------------------------------------------------------- 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
