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

Add converter for typedef property #1370

Merged
merged 5 commits into from
Feb 28, 2022
Merged

Add converter for typedef property #1370

merged 5 commits into from
Feb 28, 2022

Conversation

hyperupcall
Copy link
Contributor

@hyperupcall hyperupcall commented Feb 22, 2022

Hoi! ^w^ 👋

PR Checklist

Overview

The typedef property of TSLint is now considered during the conversion. All config options except call-signature and arrow-call-signature are accounted for. Options unaccounted for are mentioned in the notices array, when applicable

The linked issue and the ESLint typedef docs mentions that explicit-function-return-type and explicit-module-boundary-types should be used when converting call-signature and arrow-call-signature. I did that in a separate branch here, but I wasn't 100% confident that my conversion was correct, so I left it out for now. If it is close to what is considered acceptable, maybe I can add it to this PR?

I'll also add that I had to install @swc/core to get things to run, despite this mentioning its not required - not sure what's up with that. Also the Docs say Node 12 works, but 14 was the minimum LTS that worked for me (Jest threw SyntaxError: Unexpected token '.' when it encountered optional chaining)

@hyperupcall hyperupcall changed the title docs: Fix broken link to Dependencies.md Add converter for typedef property Feb 22, 2022
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.

Awesome, looks good so far. Glad to see this rule finally getting taken care of! 👏

Tests are failing (ci link) because a lack of coverage on lines 11,15,19,27,31. You'll want to add >=1 test case that covers them.

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author The PR author should address requested changes label Feb 22, 2022
@JoshuaKGoldberg
Copy link
Member

install @swc/core to get things to run

Huh, sorry about that - I'll send a separate PR to add it in.

@JoshuaKGoldberg
Copy link
Member

Node 12 works, but 14 was the minimum LTS that worked for me (Jest threw SyntaxError: Unexpected token '.' when it encountered optional chaining)

Nice, following up on that too: #1374

@hyperupcall
Copy link
Contributor Author

hyperupcall commented Feb 24, 2022

Thank you for your feedback! 😃 I will push the improved coverage and address your replies by this Friday Sunday (I have some Exams in the meantime)!

@JoshuaKGoldberg
Copy link
Member

Good luck on the exams! ❤️

This also uses `Set` to improve time complexity of searching to `O(n)`
time
This adds full support to `explicit-function-return-type` and
`explicit-module-boundary-type`
@hyperupcall
Copy link
Contributor Author

I've implemented the changes, including use of Set(), Object.entries(), and got back to 100% codecov 😄

Also, both explicit-function-return-type and explicit-module-boundary-types are now used in the conversion process. Since I'm not super familiar with TSLint, I manually tested each one to be 100% sure conversion was the same. When this conversion is done, I added a message to to notices since ESLint seems to only lint all functions (not classifying between arrow functions and non-arrow functions as TSLint does with arrow-call-signature and call-signature. I tried my best to make it succinct, lemmie know if you have any improvements

Thank you for your patience, and let me know if I missed anything or should make any other changes! ^w^

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.

Super, thanks so much @hyperupcall! 🙌

@JoshuaKGoldberg JoshuaKGoldberg merged commit 8441504 into typescript-eslint:main Feb 28, 2022
@JoshuaKGoldberg JoshuaKGoldberg removed the status: waiting for author The PR author should address requested changes label Feb 28, 2022
@JoshuaKGoldberg
Copy link
Member

This should be available in tslint-to-eslint-config@2.12.0.

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

Successfully merging this pull request may close these issues.

Missing Converter for typedef
2 participants