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 func-style ESLint rule #1421

Merged
merged 1 commit into from
Oct 22, 2020
Merged

Add func-style ESLint rule #1421

merged 1 commit into from
Oct 22, 2020

Conversation

ehmicky
Copy link
Contributor

@ehmicky ehmicky commented Oct 16, 2020

This adds the func-style ESLint rule.

@ehmicky ehmicky added the type: chore work needed to keep the product and development running smoothly label Oct 16, 2020
@ehmicky ehmicky requested a review from erezrokah October 16, 2020 13:54
@ehmicky ehmicky requested a review from a team as a code owner October 16, 2020 13:54
@ehmicky ehmicky self-assigned this Oct 16, 2020
@@ -152,40 +150,43 @@ async function pickTemplate() {
},
})
return chosentemplate
function filterRegistry(registry, input) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those functions can be safely moved to the top-level scope.

Copy link
Contributor

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

@ehmicky ehmicky force-pushed the chore/lint-func-style branch from d9696fb to 74a2e21 Compare October 20, 2020 20:40
@ehmicky
Copy link
Contributor Author

ehmicky commented Oct 20, 2020

That option is always enabled when the rule is set to expression (the default), i.e. this PR allows both styles: const myFunc = function(...){...} and const myFunc = (...) => {...}).

Both styles are almost the same and work IMHO. However, do you think it would it be useful for consistency to enforce one of them over the other?

If so, which ESLint rule would be the best to enforce this? I have struggled with this specific linting problem in my personal projects, but there might be a solution. I have even opened an issue to ESLint repository in April about this :)
However I have tried to enforce the first style (no arrow function). Maybe the second style (arrow function) is easier to enforce. no-restricted-syntax might do the trick.

@erezrokah
Copy link
Contributor

That option is always enabled when the rule is set to expression (the default),

Cool, didn't notice that.

I would also prefer to choose one of the two styles. Don't think it is a blocker for this PR (especially if we don't have built in support for it).

@ehmicky ehmicky force-pushed the chore/lint-func-style branch from 74a2e21 to 15c6fac Compare October 22, 2020 14:06
@ehmicky ehmicky requested a review from erezrokah October 22, 2020 14:06
@ehmicky
Copy link
Contributor Author

ehmicky commented Oct 22, 2020

Rebased. Ready for another review 👍

It looks like there might be a way to enforce the function declaration style using no-restricted-syntax, but it might be better to experiment this in another PR.

@ehmicky ehmicky force-pushed the chore/lint-func-style branch from 15c6fac to 995e025 Compare October 22, 2020 14:08
@ehmicky ehmicky merged commit 0413ac8 into master Oct 22, 2020
@ehmicky ehmicky deleted the chore/lint-func-style branch October 22, 2020 15:11
@ehmicky
Copy link
Contributor Author

ehmicky commented Oct 22, 2020

This is enforceable with no-restricted-syntax. However, enforcing arrow functions would also require writing:

module.exports = {
  onBuild() {...}
}

As:

module.exports = {
  onBuild: () => {...}
}

Which might be more verbose?

Also, this would not come with autofixing, so I am thinking this specific rule might be more of a nuisance than a help there, and leaving inconsistent style might be more practical. What do you think?

@erezrokah
Copy link
Contributor

Also, this would not come with autofixing, so I am thinking this specific rule might be more of a nuisance than a help there, and leaving inconsistent style might be more practical. What do you think?

Agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants