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

Updated twoslasher custom tag matcher to allow whitespace #3099

Open
wants to merge 1 commit into
base: v2
Choose a base branch
from

Conversation

Masstronaut
Copy link

Currently, there is a bug in the custom tag matcher which doesn't show up in the commented regex example due to a subtle difference between the example regex and the production code regex. The linked regex sample ( for convenience: https://regex101.com/r/8B2Wwh/1 ) differs from the production code in a subtle way: it omitted the starting ^ used in the production regex. This subtle difference means that in the linked example, comments indented with spaces or tabs are still matched. However, the production regex begins with /^\/\/, which will not match comments that have been indented as it requires // be the first characters of the line. Here's an example regex showing that an indented commented fails to match: https://regex101.com/r/fkqmfp/1

This is an issue however because it is valid and common for a comment to be intended to line up with the code being commented. In fact, this is a standard behavior of most automated formatting tools.

With this change, custom tag comments that are indented will be matched by the valuedConfigRegexp: see https://regex101.com/r/0pHWps/1

I believe the regex comment above this change should also be updated, but I assume there is an official regex101 account which should be used to create it to ensure a 3rd party (ie: me) can't delete it. If there isn't an official microsoft/typescript regex101 account please let me know and I'd be happy to create one and add the link in this pr.

Currently, there is a bug in the custom tag matcher which doesn't show up in the commented regex example due to a subtle difference between the example regex and the production code regex. The linked regex sample ( for convenience: https://regex101.com/r/8B2Wwh/1 ) differs from the production code in a subtle way: it omitted the starting `^` used in the production regex. This subtle difference means that in the linked example, comments indented with spaces or tabs are still matched. However, the production regex begins with `/^\/\/`, which will not match comments that have been indented as it requires `//` be the first characters of the line. Here's an example regex showing that an indented commented fails to match: https://regex101.com/r/fkqmfp/1

This is an issue however because it is valid *and common* for a comment to be intended to line up with the code being commented. In fact, this is a standard behavior of most automated formatting tools.

With this change, custom tag comments that are indented will be matched by the `valuedConfigRegexp`: see https://regex101.com/r/0pHWps/1
@jakebailey
Copy link
Member

I'm not sure I understand why this is a correct change; twoslash comments for values like this are expected to be unindented, i.e. at the top level. What bug does this fix? Do you have a link to where this is breaking you?

@Masstronaut
Copy link
Author

@jakebailey I've found this breaks, and also provides a peculiar and unintuitive editing experience.

Working example of using @warn:

```ts twoslash
function login(user: UserLogin) {
	const authenticatedUser = authnUser(user);
// @warn: yikes! user isn't an `AuthenticatedUser` yet generates no warning/error
	performSecureAction(user);
	//                  ^^^^
}
```

Rendered output:

image

Breaking example

When I save the code in an editor/project with automatic formatting, the comment is indented to match the block scope. This indented comment is what gets written to disk:

```ts twoslash
function login(user: UserLogin) {
	const authenticatedUser = authnUser(user);
	// @warn: yikes! user isn't an `AuthenticatedUser` yet generates no warning/error
	performSecureAction(user);
	//                  ^^^^
}
```

Now the // @warn: comment is indented, so the current twoslash regex will not match it, twoslash won't perform a transformation on it, and my final output is rendered as a regular comment instead of a warn:

image

My expectation is that indenting the comment shouldn't change the behavior, and twoslash should process the indented comment to insert a custom tag. TypeScript doesn't have different behavior based on whether or not my comments are indented, so it was surprising and unexpected behavior to discover that twoslash does have different behavior depending on if a comment is indented or not.

Copy link
Contributor

@Masstronaut the command you issued was incorrect. Please try again.

Examples are:

@ agree

and

@ agree company="your company"

@jakebailey
Copy link
Member

Not accepting indents for these config lines is how twoslash is designed, though; this is how it works in TS's own testing, and is also why we disable formatting for these snippets on the site.

@Masstronaut
Copy link
Author

Masstronaut commented Jun 2, 2024

That seems inconsistent with other ways of annotating code via comments that do support being indented, notably:

  • Highlights
  • Completions

Both highlights and completions are positionally sensitive (specific characters should be highlighted and completion should occur at an exact position). They handle indentation gracefully, which makes it even more unintuitive that the custom tags don't since custom tags aren't sensitive to position on the line.

Is there some sort of drawback to allowing custom tags to be indented?

@jakebailey
Copy link
Member

Yes, unrelated comments being treated as config lines, and inconsistency with other tooling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants