-
Notifications
You must be signed in to change notification settings - Fork 887
[quotemark] Excuse more backtick edge cases #4642
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little confused about the 2.7.1 change. Some discussion (preferably in an issue) would be good.
src/rules/quotemarkRule.ts
Outdated
|
||
return ( | ||
// In typescript below 2.7.1, don't change values either | ||
ts.version < "2.7.1" || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Directly checking ts.version
is a little unusual... maybe there's a cleaner way? Thoughts @adidahiya?
Also, @ericbf, could you elaborate on why this is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typescript below the listed version fails compilation (or otherwise misbehaves) when backticks are used in some places where it succeeds in future versions. For instance, typeof str === `string`
does not narrow the type of str
in TS versions below 2.7.1, causing compilation errors if you try to use it as a string later. In the other cases, older TS throws errors where NoSubstitutionLiterals are used instead of single/double quoted strings, but works fine in above 2.7.1; rather than cripple the rule to support order versions, I split it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created corresponding issue, #4645
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This LGTM but before merging I'd like to hear from adidahiya on whether there's a cleaner way to check on these things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's unusual and should be avoided if possible, but this seems like a reasonable use case for checking ts.version.
However, can you please use the semver package and the helper function from parse.ts
to check version ranges instead?
import { lt } from "semver";
import { getNormalizedTypescriptVersion } from "../../verify/parse";
function hasOldTscBacktickBehavior() {
return lt(getNormalizedTypescriptVersion(), "2.7.1");
}
Any update here? @adidahiya? @JoshuaKGoldberg? |
Any update here? |
This edge cases were previously flagged when they should be ignored, as changing them breaks typescript. This commit makes it so they are ignored. It also organizes a little better, using functions instead of multi-layered conditionals (it was getting confusing).
This commit adds different tests for different version ranges, as those versions behave differently.
src/rules/quotemarkRule.ts
Outdated
|
||
return ( | ||
// In typescript below 2.7.1, don't change values either | ||
ts.version < "2.7.1" || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's unusual and should be avoided if possible, but this seems like a reasonable use case for checking ts.version.
However, can you please use the semver package and the helper function from parse.ts
to check version ranges instead?
import { lt } from "semver";
import { getNormalizedTypescriptVersion } from "../../verify/parse";
function hasOldTscBacktickBehavior() {
return lt(getNormalizedTypescriptVersion(), "2.7.1");
}
Also if you merge master you'll get the new required CI check, "clean-lockfile" |
This edge cases were previously flagged when they should be ignored, as changing them breaks typescript. This commit makes it so they are ignored. It also organizes a little better, using functions instead of multi-layered conditionals (it was getting confusing).
PR checklist
Overview of change:
I keep finding other edge cases that I previously missed. I think this is all of them now, but we shall see. Related to #4588.
Is there anything you'd like reviewers to focus on?
See if y'all find any other edge cases.
CHANGELOG.md entry:
[bugfix] Excuse more
quotemark
backtick edge cases and fix behavior for TS < 2.7.1