Skip to content

Add obsolete rule converter #1063

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.

Sign up for GitHub

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
Closed
wants to merge 1 commit into from
Closed

Add obsolete rule converter #1063

wants to merge 1 commit into from

Conversation

KingDarBoja
Copy link
Collaborator

Used for prefer-inline-decorators and import-destructuring-spacing converters

PR Checklist

Overview

Also includes a obsolete rule function and test stub for making it easier to use.

@KingDarBoja
Add obsolete rule converter
ddd0fd9 
Used for prefer-inline-decorators and import-destructuring-spacing converters
@KingDarBoja KingDarBoja added the status: waiting for reviewer Waiting for a maintainer to review label May 17, 2021
@KingDarBoja KingDarBoja self-assigned this May 17, 2021
JoshuaKGoldberg
JoshuaKGoldberg requested changes May 18, 2021
View reviewed changes
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.

Love it. Just the one request, I think it's a little odd to use "" when what we really want is for the rule name to not exist.

src/converters/lintConfigs/rules/obsoleteRule.stubs.ts
expect(result).toEqual({
rules: [
{
ruleName: "",
Copy link
Member

Choose a reason for hiding this comment

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

Ha, I guess it does make sense that we would want to output something that has no output rule.

How difficult would it be to make the ruleName an optional string?

Suggested change
ruleName: "",

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was awaiting for your suggestion, I guess there is no harm making it optional? :D Haven't tried it and got lazy at the time being.

Copy link
Member

Choose a reason for hiding this comment

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

@KingDarBoja do you plan on adding that in? I can if you don't have the time!

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm playing around with this and it looks like it's a non-trivial. In convertRules.ts the output rules from the converters are stored in a converted map that stores by output ESLint name.

I'll try spinning up a changeset that shows not outputting any output ESLint rules for obsolete TSLint inputs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!

@JoshuaKGoldberg JoshuaKGoldberg added status: waiting for author The PR author should address requested changes and removed status: waiting for reviewer Waiting for a maintainer to review labels May 18, 2021
This was referenced Jul 3, 2021
Closed
Closed
Closed
Closed
Merged
@JoshuaKGoldberg
Copy link
Member

#1148 was merged; closing this in favor of it. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg requested changes

Assignees

@KingDarBoja KingDarBoja

Labels
status: waiting for author The PR author should address requested changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing Codelyzer (Angular) rule: prefer-inline-decorator
2 participants
@KingDarBoja @JoshuaKGoldberg