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

Jump to bottom

fix: improve token types and add missing type guards #1497

Merged
merged 8 commits into from Jan 24, 2020
Merged

fix: improve token types and add missing type guards #1497

merged 8 commits into from Jan 24, 2020

Conversation

armano2
Copy link
Member

@armano2 armano2 commented Jan 22, 2020 •
edited

@armano2 armano2 self-assigned this Jan 22, 2020
@typescript-eslint

This comment has been minimized.

Sign in to view
@armano2 armano2 added enhancement New feature or request package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin package: typescript-estree Issues related to @typescript-eslint/typescript-estree labels Jan 22, 2020
@armano2 armano2 mentioned this pull request Jan 23, 2020
Merged
@armano2 armano2 changed the title feat: improve token types and add missing type guards fix: improve token types and add missing type guards Jan 23, 2020
bradzacher
bradzacher approved these changes Jan 23, 2020
View reviewed changes
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.

a few questions/comments, otherwise lgtm

packages/eslint-plugin/typings/eslint-utils.d.ts
@@ -113,67 +113,67 @@ declare module 'eslint-utils' {

export function isArrowToken(
token: TSESTree.Token | TSESTree.Comment,
): boolean;
): token is TSESTree.PunctuatorToken & { value: '=>' };
Copy link
Member

Choose a reason for hiding this comment

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

question:
did we want to make this generic on the value?

Copy link
Member Author

Choose a reason for hiding this comment

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

i did that at begging but typescript was behaving in unexpected way and type-guard was not working as it should

packages/experimental-utils/src/ts-eslint/SourceCode.ts Show resolved Hide resolved
packages/typescript-estree/src/convert-comments.ts Show resolved Hide resolved
packages/typescript-estree/src/ts-estree/ts-estree.ts Show resolved Hide resolved
packages/typescript-estree/src/ts-estree/ts-estree.ts Show resolved Hide resolved
bradzacher
bradzacher requested changes Jan 23, 2020
View reviewed changes
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.

oops forgot to rc

@armano2
Copy link
Member Author

armano2 commented Jan 23, 2020

after merge i fixed up types used in recently added comma-spacing rule

@codecov
Copy link

codecov bot commented Jan 23, 2020 •
edited

Codecov Report

Merging #1497 into master will increase coverage by 0.01%.
The diff coverage is 97.87%.

@@            Coverage Diff            @@
##           master   #1497      +/-   ##
=========================================
+ Coverage   95.58%   95.6%   +0.01%     
=========================================
  Files         147     148       +1     
  Lines        6617    6664      +47     
  Branches     1891    1909      +18     
=========================================
+ Hits         6325    6371      +46     
  Misses        111     111              
- Partials      181     182       +1
Impacted Files Coverage Δ
...ges/experimental-utils/src/ts-eslint/SourceCode.ts 100% <ø> (ø) ⬆️
packages/eslint-plugin/src/util/astUtils.ts 87.5% <ø> (ø) ⬆️
packages/eslint-plugin/src/rules/index.ts 100% <100%> (ø) ⬆️
packages/eslint-plugin/src/rules/comma-spacing.ts 97.82% <97.82%> (ø)

bradzacher
bradzacher reviewed Jan 23, 2020
View reviewed changes
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.

everything you've got so far LGTM.
one final request while you're at it - there's a few functions in eslint-plugin/src/utils, could you give them the same treatment too please?

After that, happy to merge.

@armano2
Copy link
Member Author

armano2 commented Jan 23, 2020 •
edited

ok, done

we are still missing type-guards for negated versions of those functions

@armano2
Copy link
Member Author

armano2 commented Jan 24, 2020

i did small digging into negated version of those functions and it seems really hard to make them actually work, microsoft/TypeScript#29317 could help with that

bradzacher
bradzacher approved these changes Jan 24, 2020
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment •
edited

Choose a reason for hiding this comment

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

LGTM - thanks for the cleanup.
I'm not worried about the negated functions. We can deal with them at a later point

@bradzacher bradzacher merged commit ce41d7d into typescript-eslint:master Jan 24, 2020
@armano2 armano2 deleted the type-improvement branch January 24, 2020 01:08
@armano2

This comment has been minimized.

Sign in to view
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bradzacher bradzacher bradzacher approved these changes

Assignees

@armano2 armano2

Labels
enhancement New feature or request package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@armano2 @bradzacher