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

fix(typescript-estree): support private optional property definition #3997

Conversation

sosukesuzuki
Copy link
Contributor

Supports private optional property definition

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @sosukesuzuki!

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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #3997 (8b58782) into master (41d6cac) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3997      +/-   ##
==========================================
- Coverage   93.40%   93.38%   -0.02%     
==========================================
  Files         151      168      +17     
  Lines        8093     9506    +1413     
  Branches     2568     2943     +375     
==========================================
+ Hits         7559     8877    +1318     
- Misses        181      237      +56     
- Partials      353      392      +39     
Flag Coverage Δ
unittest 93.38% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/typescript-estree/src/convert.ts 98.28% <ø> (ø)
packages/typescript-estree/src/ts-estree/index.ts 100.00% <0.00%> (ø)
...escript-estree/src/semantic-or-syntactic-errors.ts 87.50% <0.00%> (ø)
...-estree/src/create-program/createProjectProgram.ts 94.11% <0.00%> (ø)
packages/typescript-estree/src/convert-comments.ts 100.00% <0.00%> (ø)
...ckages/typescript-estree/src/jsx/xhtml-entities.ts 100.00% <0.00%> (ø)
packages/typescript-estree/src/node-utils.ts 97.83% <0.00%> (ø)
packages/typescript-estree/src/ast-converter.ts 100.00% <0.00%> (ø)
...ript-estree/src/create-program/createSourceFile.ts 87.50% <0.00%> (ø)
packages/typescript-estree/src/version-check.ts 100.00% <0.00%> (ø)
... and 8 more

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for taking this on @sosukesuzuki! Just requesting a couple more test cases to make sure it works well with different optional initial values.

@JoshuaKGoldberg
Copy link
Member

By the way -- most PRs generally are asked to correspond to an existing issue to make make sure the change is something that would be accepted before you spend too much time working on it. This one is a definite win IMO, so just a heads up for next time. 🙂

(echoing from the other thread for visibility)

@bradzacher bradzacher added bug Something isn't working awaiting response Issues waiting for a reply from the OP or another party labels Oct 14, 2021
@sosukesuzuki sosukesuzuki force-pushed the support-optional-private-property branch from d689829 to 8b58782 Compare October 14, 2021 06:03
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Oct 14, 2021
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noice - thanks for fixing this!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants