Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: New rule to check for properly escaping "<" and ">" characters #864

Closed
iliubinskii opened this issue Apr 5, 2022 · 4 comments · Fixed by #930
Closed

Comments

@iliubinskii
Copy link

Motivation

/**
 * Returns Set<string>.
 *
 * @returns Set<string>.
 */
export function f(): Set<string> {
  return new Set([]);
}

VSCode does not show ""
image

/**
 * Returns Set\<string\>.
 *
 * @returns Set\<string\>.
 */
export function f(): Set<string> {
  return new Set([]);
}

The escaped version works:
image

Current behavior

Escaping "<" and ">" is not checked.

Desired behavior

New rule to enforce escaping "<" and ">".

@brettz9
Copy link
Collaborator

brettz9 commented Apr 5, 2022

Is this not a bug of VSC? Why is there any need for escaping with backslashes? This does not seem part of JSDoc that I'm aware of either...

@iliubinskii
Copy link
Author

This is not vscode bug.

Here is typescript playground example:
https://www.typescriptlang.org/play#code/PQKhCgAIUgRB7AxgVwLYFMB2AXAhtgS3kyhgCV1F4AnAEwB4AVAPgDpThx0APABxuyQAZskyJCxYQAoAlAC5IAN3gFakAN4BfIA

image

I guess that <...> in documentation comments is interpreted as HTML tag = it is not shown as plain text.

If you don't find it useful to add new rule in your plugin - no problem. I can write my own rule for this.

@brettz9
Copy link
Collaborator

brettz9 commented Apr 8, 2022

I see. So we're deliberately not talking about within types here--only the descriptions.

Note that, like for XML, one doesn't actually need to escape >, but one does need to escape < (and in the event one wishes to have a numeric character reference show up, one has to escape ampersands too, e.g., \&#xabcd; instead of &#abcd;.

Note too that one can use any of the XML/HTML5 predefined entities (&amp;, &lt;, &gt;, &quot;, &apos;) like &amp;#xabcd;.

And interestingly, one can also use yet another escape mechanism, the Markdown-style backticks (e.g., `&#xabcd;`).

This is somewhat related to #710 (though yours is as a new rule and doesn't relate to that particular escaping problem).

There is kind of a problem as far as the latter not apparently being fully settled on per microsoft/tsdoc#166 .

However, I'm starting to think it is a good idea to at least allow configurable support for such escapes, even though I'd really prefer if a standard were coming that we could wait for. But I think we should go with configuration to support which is effectively the de facto standard now, TypeScript (and its tooling).

One trick for this latter problem is that the backslash in *\/ does not mean that a visible backslash needs to be doubled everywhere (you do need to double it to *\\/ if you want to show *\/). Also if you do double it (anywhere), it will only show once. Likewise, backticks do not need to be double-escaped to show a literal.

As to a course of action, I think this proposed rule should have configuration as to whether < and & should require escaping, and configuration for the fixer to decide which kind to apply (backslashes or predefined entities at least). Auto-adding backticks would require grabbing until the end of the sequence so as to encapsulate the whole, e.g., &amp; to `&amp;` and might be problematic, as with backslashes, if found immediately after another of its kind, e.g., the unescaped `&#xabc; would become the still unescaped ``&#xabc;`). But if we note that backticks and backslashes are potentially problematic for auto-fixing in this way we could still offer them since not everyone will want to use the uglier predefined entities. I don't think we need to have code to require escaping of > (nor of course for " or ').

Not everyone will want this rule, however, as sometimes one intentionally wants to use HTML elements. We could in theory also provide an option to auto-escape backslashes or backticks to avoid unintended escaping, but that would probably not be desired by most (and fixers would need one mechanism).

brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Oct 30, 2022
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Oct 31, 2022
@brettz9 brettz9 closed this as completed in 1776e18 Nov 1, 2022
@gajus
Copy link
Owner

gajus commented Nov 1, 2022

🎉 This issue has been resolved in version 39.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Nov 1, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Nov 2, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint-plugin-jsdoc](https://github.com/gajus/eslint-plugin-jsdoc) | devDependencies | minor | [`39.4.0` -> `39.5.0`](https://renovatebot.com/diffs/npm/eslint-plugin-jsdoc/39.4.0/39.5.0) |

---

### Release Notes

<details>
<summary>gajus/eslint-plugin-jsdoc</summary>

### [`v39.5.0`](https://github.com/gajus/eslint-plugin-jsdoc/releases/tag/v39.5.0)

[Compare Source](gajus/eslint-plugin-jsdoc@v39.4.0...v39.5.0)

##### Features

-   `text-escaping` rule; fixes [#&#8203;864](gajus/eslint-plugin-jsdoc#864) ([1776e18](gajus/eslint-plugin-jsdoc@1776e18))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMTIuMSJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1622
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants