Skip to content

Add missing converter: file-name-casing #204

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

Conversation

da-snap
Copy link
Contributor

@da-snap da-snap commented Oct 4, 2019

Linked to issue: #161

PR Checklist

Overview

Add converter for file-name-casing. Map casings to new rule in the unicorn plugin.

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.

Looking good so far! A few changes before merging, please.

Comment on lines 18 to 22
const casesMap = new Map();
casesMap.set("camel-case", "camelCase");
casesMap.set("pascal-case", "pascalCase");
casesMap.set("kebab-case", "kebabCase");
casesMap.set("snake-case", "snakeCase");
Copy link
Member

Choose a reason for hiding this comment

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

You can make a map with initialized properties in the constructor:

new Map([
    ["camel-case", "camelCase"],
    // ...
]);

That being said, I don't think this really needs to be a Map. Only .get is used on it, and always for keys guaranteed to exist. Can you switch it to just a regular object, and initialize it outside of collectArguments (since it's only assigned to and never modified)?

@@ -0,0 +1,56 @@
import { RuleConverter } from "../converter";

const IGNORE_CASE_NOTICE = "ESLint (Unicorn plugin) does not support ignore as case";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const IGNORE_CASE_NOTICE = "ESLint (Unicorn plugin) does not support ignore as case";
const IGNORE_CASE_NOTICE = "ESLint (Unicorn plugin) does not support the 'ignore' case.";

...to be a little more clear. Other notices end with periods too.

casesMap.set("pascal-case", "pascalCase");
casesMap.set("kebab-case", "kebabCase");
casesMap.set("snake-case", "snakeCase");
const foundCases: { [k: string]: any } = {};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const foundCases: { [k: string]: any } = {};
const foundCases: { [k: string]: string } = {};

...right? Once casesMap is converted to a { [i: string]: string }, this should work more easily in the typings.

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author The PR author should address requested changes label Oct 4, 2019
* Add notice for changed behavior
* Change map to object
* Correct notice about the 'ignore' case
* Specify map typing
@da-snap
Copy link
Contributor Author

da-snap commented Oct 5, 2019

Thank you for your feedback! I tried to fix all issues. Let me know if there is anything else.

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.

Great. Thanks for the changes!

@JoshuaKGoldberg JoshuaKGoldberg merged commit 07be509 into typescript-eslint:master Oct 5, 2019
@da-snap da-snap deleted the 161-file-name-casing-converter branch October 7, 2019 08:22
@JoshuaKGoldberg JoshuaKGoldberg removed the status: waiting for author The PR author should address requested changes label Oct 10, 2019
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: file-name-casing
2 participants