-
Notifications
You must be signed in to change notification settings - Fork 0
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
Ensure use of block body for arrow functions #1006
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cruikshanks
force-pushed
the
ensure-block-body-for-arrow-functions
branch
from
May 12, 2024 22:10
b98d469
to
563dc72
Compare
This will mean the exception in our conventions will have to go. But if consistency was the name of the game we probably shouldn't have had the exception in the first place 😬 |
Cruikshanks
requested review from
Demwunz,
robertparkinson,
Jozzey,
jonathangoulding,
Beckyrose200 and
rvsiyad
May 12, 2024 22:17
jonathangoulding
previously approved these changes
May 13, 2024
robertparkinson
previously approved these changes
May 13, 2024
Jozzey
reviewed
May 13, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. You seem to have upset SonarCloud a bit though.
DEFRA/water-abstraction-team#115 Just like [parens in arrow functions](#1004) how you write an [arrow function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions) can differ. ```javascript const materials = [ 'Hydrogen', 'Helium', 'Lithium', 'Beryllium' ] // Block body materials.forEach((material) => { console.log(`${material} - ${material.length}`) }) // Concise body materials.forEach((material) => console.log(`${material} - ${material.length}`)) ``` The _block-body_ version can be applied in all cases. But like with parens where devs will often drop the parens when there is only 1 param, they'll use _concise_ for simple arrow functions and _block-body_ for the rest. To us though, the argument is the same. For those new or inexperienced with JavaScript this inconsistency can be confusing and a blocker to progressing with the language. The more we can do to keep the code base simple and consistent, the easier new folks will find working with it. So, this change enables [the rule in ESLint](https://eslint.org/docs/latest/rules/arrow-body-style) that requires all arrow-functions to use the block-body.
Cruikshanks
dismissed stale reviews from robertparkinson and jonathangoulding
via
May 13, 2024 15:12
f9bfed9
Cruikshanks
force-pushed
the
ensure-block-body-for-arrow-functions
branch
from
May 13, 2024 15:12
018dbe5
to
f9bfed9
Compare
We are using the [ESLint stylistic plugin](https://eslint.style/) and it has a related rule; [https://eslint.style/rules/js/implicit-arrow-linebreak#when-not-to-use-it](https://eslint.style/rules/js/implicit-arrow-linebreak#when-not-to-use-it) which suggest it should not be used when `arrow-body-style` is set to 'always'.
Cruikshanks
force-pushed
the
ensure-block-body-for-arrow-functions
branch
from
May 13, 2024 15:15
f9bfed9
to
c47f473
Compare
Cruikshanks
added a commit
to DEFRA/water-abstraction-team
that referenced
this pull request
May 13, 2024
#115 We have been working on using ESLint to help highlight coding convention infractions instead of folks either having to remember them when submitting or reviewing PRs. In [Ensure use of block body for arrow functions](DEFRA/water-abstraction-system#1006) we managed to figure out how to have the tool highlight when someone has used concise over block-body format for an arrow function. However, the rule cannot distinguish between when it was being used normally, or just to check if something is 'truthy'. That was the one exception we had and there is no point in keeping it documented here if the tool won't allow. We also didn't think it was worth the effort to try and 'hack' ESLint or develop a custom rule just to support this. On balance, its goes against one of our guiding principle of consistency anyway. So, this change removes the exceptions from our docs.
Cruikshanks
added a commit
to DEFRA/water-abstraction-team
that referenced
this pull request
May 15, 2024
#115 We have been working on using ESLint to help highlight coding convention infractions instead of folks either having to remember them when submitting or reviewing PRs. In [Ensure use of block body for arrow functions](DEFRA/water-abstraction-system#1006) we managed to figure out how to have the tool highlight when someone has used concise over block-body format for an arrow function. However, the rule cannot distinguish between when it was being used normally, or just to check if something is 'truthy'. That was the one exception we had and there is no point in keeping it documented here if the tool won't allow it. We also didn't think it was worth the effort to try and 'hack' ESLint or develop a custom rule just to support this. On balance, it goes against one of our guiding principles of consistency anyway. So, this change removes the exceptions from our docs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DEFRA/water-abstraction-team#115
Just like parens in arrow functions how you write an arrow function can differ.
The block-body version can be applied in all cases. But like with parens where devs will often drop the parens when there is only 1 param, they'll use concise for simple arrow functions and block-body for the rest.
To us though, the argument is the same. For those new or inexperienced with JavaScript, this inconsistency can be confusing and a blocker to progressing with the language. The more we can do to keep the code base simple and consistent, the easier new folks will find working with it.
So, this change enables the rule in ESLint that requires all arrow functions to use the block-body.
The ESLint stylistic plugin we use has a related rule; https://eslint.style/rules/js/implicit-arrow-linebreak#when-not-to-use-it which suggests it should not be used when
arrow-body-style
is set to 'always'. This is why it is disabled as part of this change.