-
Notifications
You must be signed in to change notification settings - Fork 61
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
Support comments in arrow functions when fixing #253
Conversation
@@ -16,21 +15,31 @@ module.exports = { | |||
create(context) { | |||
const sourceCode = context.getSourceCode(); | |||
|
|||
// eslint-disable-next-line max-statements |
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.
Not sure how you feel about this rule, but I didn't see an obvious way to break up this method without passing multiple return values.
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.
It’s not a hard requirement. I don’t have a better approach in mind for now. As long as we have a good test coverage we can always refactor the code later 😉.
lib/rules/no-mocha-arrows.js
Outdated
let functionKeyword = 'function'; | ||
const beforeArrowComment = sourceCode.text.slice(beforeArrowToken.range[1], arrow.range[0]).trim(); | ||
const afterArrowComment = sourceCode.text.slice(arrow.range[1], fn.body.range[0]).trim(); |
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.
For better readability I would suggest to extract the repeated pattern of sourceCode.text.slice(start, end).trim()
to a function.
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.
Is
sliceAndTrim( sourceCode.text, beforeArrowToken.range[1], arrow.range[0] );
much more concise or legible?
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.
I think sourceCode.text
doesn’t need to be a parameter when we define the new function in the scope of the create
function. Maybe a better name would be something like extractSourceTextByRange()
. WDYT?
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.
LGTM. Thank you!
Fixes #251