support regions in @includeCode

[shawninder]

WORK IN PROGRESS / DO NOT MERGE

This is a proof of concept for supporting regions in @includeCode tags, meaning you could insert part of a file as a code block by referencing a region of that file.

Example

sourceFile.ts

console.log("Code we don't want inserted")
// #region regionName
console.log('Code we want to insert')
// #endregion regionName
console.log("More code we don't want inserted")

documents/some-page.md

Here is an example:
{@includeCode src/sourceFile.ts#regionName}
This is useful for reasons.

output:

---

Here is an example:

console.log('Code we want to insert')

This is useful for reasons.

---

Notes

• Once merged, this would close Support regions and line numbers within @includeCode tag reference #2814

@Gerrit0 if this basic strategy seems OK, I would add the following to this PR before merging:

TO DO

• Get a simple example working with Typescript
• Use the appropriate region syntax for different languages as documented in the VS Code documentation for Folding
• Make sure small variants don't break things (like using //#region NAME instead of // #region NAME)
• Support regions for Markdown files in the @include tag as well
• Produce useful (translated) errors where appropriate:
  • when a region is specified but cannot be found
  • when a region is found but empty (perhaps just a warning for this one?)
  • when a region is found more than once in a file
• Add support for line-number syntax
• Add tests to validate success, error and warning paths related to these new features
• Document the above additions in site/tags/include.md
• Use these new features in the example app (/example)
• Add an entry in the Changelog describing the changes brought about by this PR

Thoughts? Questions? Advice?

Updates

• Check all new regular expressions against the RE DOS checker
• Lint

[Gerrit0]

Yes, this seems reasonable. Might want to split the logic out into its own function at this point.

Add support for line-number syntax

Is there any case in which this would be a better option than using a region? It seems like an objectively worse UX to me...

[shawninder]

Might want to split the logic out into its own function at this point.

I'll probably be extracting a function to its own file for this, indeed.

Is there any case in which [line-number syntax] would be a better option than using a region? It seems like an objectively worse UX to me...

Agreed. I'm just thinking of the case where you want to include code from a file you don't control or in which you can't add comments to define regions (think JSON, for example, which does't support comments). In this case the line-numbers seem like the only text-based approach (as opposed to using a language-aware AST) that could work…

Can I let you decide if it's worth including?

[Gerrit0]

Ah, that's a very fair point, line numbers is probably worth it at some point

[shawninder]

The holidays are over so I'm back at it Today, but I think I already made a mistake. I tried pulling the latest changes before pushing mine but this PR is still showing more than my own changes…

I'm not sure how to make the PR clean from here… Help?

---

Other questions and comments about my actual code changes:

• I'm using regular expressions to support whitespace in the region tags, is that OK? Do I need to guard them somehow to avoid regular expression related attack vectors?
• I tried my best at naming things in line with the naming convention I think I'm seeing in the package, but I don't feel like the names I found are particularly good, especially the names of the translated error messages. Ideas?
• I'm having trouble finding existing tests for the @include and @includeCode tags. These would be helpful before I write my own tests for the new features. Any guidance would be appreciated.
• I added new English error messages. Do I need to translate them? French is the only language I could translate to unassisted…
• I created new functions in src/lib/converter/plugins/IncludePlugin.ts and a new file called src/lib/converter/utils/regionTagREsByExt.ts. Am I following the existing strategy closely enough? I could move the new functions to that new file (at the cost of extra parameters to these functions) if that's better…
• I'm using a controversial double-ternary expression in src/lib/converter/plugins/IncludePlugin.ts Would you prefer I use different syntax?

---

Thanks for any and all feedback. Looking forward to wrapping this up and marking it ready for official review and eventual merging :)

[Gerrit0]

I'm not sure how to make the PR clean from here… Help?

Not exactly sure how this happened... it looks like some kind of rebase gone wrong as Git thinks you went and modified every commit. If you run git fetch upstream and git rebase -i upstream/master, :wq, then force push, your changes should be re-applied on top of current master.

I'm using regular expressions to support whitespace in the region tags, is that OK? Do I need to guard them somehow to avoid regular expression related attack vectors?

This is fine, I usually toss new ones into https://devina.io/redos-checker to check

I tried my best at naming things in line with the naming convention I think I'm seeing in the package, but I don't feel like the names I found are particularly good, especially the names of the translated error messages. Ideas?

This looks pretty reasonable to me, I'm considering going the same route as TypeScript for these names someday which will make them automatically generated, but for now it makes sense. The names are completely subject to change anyways.

I'm having trouble finding existing tests for the @include and @includeCode tags. These would be helpful before I write my own tests for the new features. Any guidance would be appreciated.

It looks like I didn't write unit tests specifically for this as they were used in TypeDoc's site so effectively tested there. There's a test that was added later (search for 2800 in issues.c2.test.ts)

I added new English error messages. Do I need to translate them? French is the only language I could translate to unassisted…

Don't worry about adding translations, they'll just use English until someone who speaks other languages natively adds a translation.

I created new functions in src/lib/converter/plugins/IncludePlugin.ts and a new file called src/lib/converter/utils/regionTagREsByExt.ts. Am I following the existing strategy closely enough? I could move the new functions to that new file (at the cost of extra parameters to these functions) if that's better…

I probably would have left the region tag regexes in the IncludePlugin file, fine where they are though. I tend to only split things out to other files if they are useful in more than one place.

I'm using a controversial double-ternary expression in src/lib/converter/plugins/IncludePlugin.ts Would you prefer I use different syntax?

Looks fine to me!

I'll take a closer look at the code tomorrow, off to spend more time with family :)

[Gerrit0]

Apologies for the delay here! Time has a way of slipping away... not going to merge this weekend either, but it's first on my list for next weekend. This is looking really good to me. I especially like the changes you made to the docs! It seems to work well in the playing around I've done so far.

[Gerrit0]

We should regex-escape target here, there's an escapeRegExp function in src/lib/utils/general.ts

[shawninder]

Good point. Done in my latest commit.

[shawninder]

Apologies for the delay here! Time has a way of slipping away... not going to merge this weekend either, but it's first on my list for next weekend.

No worries @Gerrit0, I am in no rush :)

About the conflicts in CHANGELOG.md, do you want me to remove my changes to the changelog or merge and fix or anything else?

[Gerrit0]

I can fix it when merging :) I'll probably look at making a few unit tests as well

[Gerrit0]

I finally got around to pulling this down, writing some unit tests, and just playing with it. Found a couple issues, but it's all merged to my dev staging branch, will go to master as soon as I'm ready to push a release (I want #2831 merged too), hopefully this time it will actually be this weekend.

[phoneticallySAARTHaK]

@Gerrit0 hey, please don't make a release yet, I'd like to include the sidebar changes as well, since it might change some selectors again, and I don't wanna get cursed by theme maintainers for repeated breaking changes

[Gerrit0]

... shoot, I forgot about themes, there were only two custom ones for 0.27 published last I looked, neither of which would have been affected by your changes. Now it looks like there are more which will be affected, so that rewrite will have to come with a minor version bump.

[phoneticallySAARTHaK]

Alright, and btw, it won't be a rewrite. I'll just put the sidebar in a dialog.