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 space-before-function-paren #293

Conversation

bouzuya
Copy link
Contributor

@bouzuya bouzuya commented Dec 19, 2019

PR Checklist

Overview

See: #292

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.

LGTM, thanks for re-tackling this converter @bouzuya!

If you have time to address the requests soon, that'd be swell - otherwise I can take care of them myself.

src/rules/converters/space-before-function-paren.ts Outdated Show resolved Hide resolved
const ruleName = "space-before-function-paren";
if (argument === "always" || argument === "never") {
return { rules: [{ ruleArguments: [argument], ruleName }] };
} else if (typeof argument === "object" && argument !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

The contents in the body of this else if are a little substantial to read through. Would you mind splitting them into a separate function? That'd be helpful for readability IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry. I can't read English well. In this what you mean?

const isObject = (o: unknown): o is Record<string, "always" | "never"> =>
    typeof o === "object" && o !== null;

// ...
} else if (isObject(argument)) {
// ...

Alternatively, I think it would be better to use typeof argument === "undefined" if tslint.json is valid.

// ...
} else if (typeof argument === "undefined") {
    return { rules: [{ ruleName }] };
}
const notices = Object.keys(argument)
    .filter(option => !SUPPORTED_OPTIONS.includes(option))
    .map(option => `Option "${option}" is not supported by ESLint.`);
const filtered = Object.keys(argument)
    .filter(option => SUPPORTED_OPTIONS.includes(option))
    .reduce<Record<string, "always" | "never">>((obj, option) => {
        return { ...obj, [option]: argument[option] };
    }, {});
return {
    rules: [
        {
            ...(notices.length !== 0 && { notices }),
            ...(Object.keys(filtered).length !== 0 && { ruleArguments: [filtered] }),
            ruleName,
        },
    ],
};

What would you like to do specifically?

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry for the ambiguity in my comments! Yes, your second suggestion around using typeof seems great.

My overall goal was to reduce the amount of logic placed inside the else statement, since that can be confusing to read through. Moving that stuff into a separate function can be helpful to split the code up so each individual portion is understandable.

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author The PR author should address requested changes label Dec 21, 2019
@bouzuya bouzuya force-pushed the fix-space-before-function-paren branch 2 times, most recently from f5dbe8d to de0edc8 Compare December 22, 2019 00:01
@bouzuya bouzuya force-pushed the fix-space-before-function-paren branch from de0edc8 to d39e097 Compare December 22, 2019 00:42
@JoshuaKGoldberg JoshuaKGoldberg added status: waiting for reviewer Waiting for a maintainer to review status: waiting for author The PR author should address requested changes and removed status: waiting for author The PR author should address requested changes status: waiting for reviewer Waiting for a maintainer to review labels Dec 22, 2019
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.

As before @bouzuya - I'll apply the discussed changes soon if you don't have the time. Thanks! 🙌

@bouzuya
Copy link
Contributor Author

bouzuya commented Dec 23, 2019

Can I ask to fix them?

@JoshuaKGoldberg
Copy link
Member

Sure! I'll merge this in now and send a separate PR. Thanks for all your work! 🙌

@JoshuaKGoldberg JoshuaKGoldberg merged commit dd5138c into typescript-eslint:master Dec 24, 2019
@JoshuaKGoldberg
Copy link
Member

Actually, wait, @bouzuya I think I missed a commit - you applied all the changes I asked for. I think I'm just confusing you unnecessarily. Sorry! 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for author The PR author should address requested changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration for rule "space-before-function-paren" is invalid
2 participants