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: improve token types and add missing type guards #1497

Merged
merged 8 commits into from Jan 24, 2020

Conversation

@armano2
Copy link
Member

armano2 commented Jan 22, 2020

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

This comment was marked as resolved.

Copy link

typescript-eslint bot commented Jan 22, 2020

Thanks for the PR, @armano2!

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.

@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
Copy link
Member

bradzacher left a comment

a few questions/comments, otherwise lgtm

@@ -113,67 +113,67 @@ declare module 'eslint-utils' {

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

This comment has been minimized.

Copy link
@bradzacher

bradzacher Jan 23, 2020

Member

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

This comment has been minimized.

Copy link
@armano2

armano2 Jan 23, 2020

Author Member

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

Copy link
Member

bradzacher left a comment

oops forgot to rc

armano2 added 2 commits Jan 23, 2020
@armano2

This comment has been minimized.

Copy link
Member Author

armano2 commented Jan 23, 2020

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

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 23, 2020

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%> (ø)
Copy link
Member

bradzacher left a comment

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

This comment has been minimized.

Copy link
Member Author

armano2 commented Jan 23, 2020

ok, done

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

@armano2

This comment has been minimized.

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

Copy link
Member

bradzacher left a comment

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
7 checks passed
7 checks passed
Semantic Pull Request ready to be squashed
Details
codecov/patch 97.87% of diff hit (target 90%)
Details
codecov/project 95.6% (+0.01%) compared to da69d34
Details
typescript-eslint.typescript-eslint Build #20200123.12 succeeded
Details
typescript-eslint.typescript-eslint (Primary code validation and tests) Primary code validation and tests succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_10_x) Run unit tests on other Node.js versions node_10_x succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_8_x) Run unit tests on other Node.js versions node_8_x succeeded
Details
@armano2 armano2 deleted the armano2:type-improvement branch Jan 24, 2020
@armano2

This comment was marked as outdated.

Copy link
Member Author

armano2 commented Jan 24, 2020

actually i found a way to make them work :>

i will make follow up pr with my findings

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.