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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add opt-in flag to convert `tslint:disable` comments to `eslint-disable` #136

Open
JoshuaKGoldberg opened this issue Aug 5, 2019 · 5 comments 路 May be fixed by #246
Open

Add opt-in flag to convert `tslint:disable` comments to `eslint-disable` #136

JoshuaKGoldberg opened this issue Aug 5, 2019 · 5 comments 路 May be fixed by #246

Comments

@JoshuaKGoldberg
Copy link
Collaborator

@JoshuaKGoldberg JoshuaKGoldberg commented Aug 5, 2019

馃殌 Feature Request

Per a comment in sourcegraph/sourcegraph#5072 - it'd be swell to automate that process as well. tslint-to-eslint-config already has data from rule mergers, so when the config is generated, mapping through tslint-disable comments can use the same information generated by them.

Existing Behavior

Nothing happens.

Change Proposal

Add an optional CLI flag that takes in a glob or globs of source files to convert, and changes tslint:disable-next-line: no-floating-promises to the equivalent eslint-disable-next-line: @typescript-eslint/no-floating-promises lines.

@KingDarBoja

This comment has been minimized.

Copy link
Contributor

@KingDarBoja KingDarBoja commented Oct 20, 2019

@JoshuaKGoldberg I think I have a basic setup but not sure how to test it without relying on making a separate test file since I am not confident if it gonna work. You can check out this branch what I have done so far.

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Collaborator Author

@JoshuaKGoldberg JoshuaKGoldberg commented Oct 20, 2019

@KingDarBoja That looks like a great start. Now that you mention it, it does seem like a ts.createSourceFile inside is the only way to know what's a real disable comment... fun. 馃槄

For testing it, a good start might be to try splitting convertComments into >=3 files:

  • Exported function that creates a ts.SourceFile for a given file - either including the file system shenanigans, or with that split up into a separate files
  • Exported function that finds all matching comments in a source file
  • Exported function that, given a regexp comment match, finds the equivalent ESLint rule name for it

YMMV. 馃殫

Btw, maybe --convertComments for the CLI flag?

@KingDarBoja

This comment has been minimized.

Copy link
Contributor

@KingDarBoja KingDarBoja commented Oct 20, 2019

@JoshuaKGoldberg Don't worry, I made it from the current source code by calling the node ../tslint-to-eslint-config/bin/tslint-to-eslint-config -c command on some test folder and so far, I am getting the comments but still dealing with the for loop logic.

I will push new changes in few minutes, ahd to fix the regex pattern and the source dirname, in the meantime, look at the output:
testing convert comments

I am properly extracting the source file regex match and I do have some information of where the comments are located. Just as extra info, my test file is a copy-paste of the convertComments code with some tslint comments 馃槅

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Collaborator Author

@JoshuaKGoldberg JoshuaKGoldberg commented Oct 20, 2019

it's so beautiful

@KingDarBoja

This comment has been minimized.

Copy link
Contributor

@KingDarBoja KingDarBoja commented Oct 20, 2019

it's so beautiful

I fixed already the regex pattern, at the end, had to play a little with it because I am not so skilled with it. Now we get the proper for-loop check and we can do whatever we want inside it.

Now the next thing would be the rule change logic and overwriting the file with the proper path (which looks like it is not being taken into account).

Cheers!

@KingDarBoja KingDarBoja linked a pull request that will close this issue Oct 20, 2019
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can鈥檛 perform that action at this time.