-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(typescript-estree): ensure consistent TSMappedType AST shape #11086
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
fix(typescript-estree): ensure consistent TSMappedType AST shape #11086
Conversation
Thanks for the PR, @dbarabashh! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
View your CI Pipeline Execution ↗ for commit 656a3f9.
☁️ Nx Cloud last updated this comment at |
readonly: node.readonlyToken | ||
? node.readonlyToken.kind === SyntaxKind.ReadonlyKeyword || | ||
getTextForTokenKind(node.readonlyToken.kind) | ||
: undefined, |
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.
Note that
readonly
is an enum oftrue
,+
and-
. So previously I proposedundefined
as the default value. I guessfalse
does no harm here.
Agreed that either could work, and also agreed on going with undefined
. That feels to me like a more normal default when there are multiple truthy options ('+'
, '-'
). I can see why false
might make sense as a counterpoint to true
, but visually I think it makes less sense than undefined
.
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.
🙌 The source code change looks good to me, thanks! Requesting changes on going with the ast-spec tests that other types of syntax conversions are verified with too.
@@ -449,5 +449,79 @@ describe('convert', () => { | |||
expect(tsMappedType.typeParameter).toBeUndefined(); | |||
expect(Object.keys(tsMappedType)).toContain('typeParameter'); | |||
}); | |||
|
|||
describe('TSMappedType AST shape consistency', () => { |
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.
[Testing] We normally put fixtures in packages/ast-spec/src
. Is there a reason not to here?
If not, I think packages/ast-spec/src/type/TSMappedType
would be the right place to add new fixtures around readonly
and/or optional mapped types.
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (25.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #11086 +/- ##
==========================================
+ Coverage 90.82% 90.84% +0.01%
==========================================
Files 497 497
Lines 50204 50320 +116
Branches 8274 8311 +37
==========================================
+ Hits 45600 45714 +114
- Misses 4589 4591 +2
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
3c36704
to
6a655f2
Compare
6a655f2
to
76f693f
Compare
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 -- just because my review time is limited I took a moment to fix up your fixtures (you had nested the folders incorrectly which meant the snapshots weren't generated -- we might wanna add an error for this...). And I added a few extra snapshots to cover some more cases.
this all LGTM!
@bradzacher Thank you so much. I was just about to write a comment about the file structure. and one more question, what should we do about the coverage rate? |
#6116 strikes again! We can ignore the coverage drop in this PR. |
4a97721
into
typescript-eslint:main
PR Checklist
Overview
Example a:
{ [k in any]: any }
Example b:
{ [k in any]?: any }
Example c:
{ readonly [k in any]: any }
Example d:
{ +readonly [k in any]?: any }
Example e:
{ -readonly [k in any]: any }