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: implicit-arrow-linebreak adds extra characters (fixes #11268) #11407

Merged
merged 12 commits into from Mar 15, 2019
Merged

Fix: implicit-arrow-linebreak adds extra characters (fixes #11268) #11407

merged 12 commits into from Mar 15, 2019

Conversation

ghost
Copy link

@ghost ghost commented Feb 16, 2019

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ x ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Fixes 11268

What changes did you make? (Give an overview)
The original auto-fix calculated white spaces into formatting comments and parentheses, which was the cause for extra characters.

The cause for extra parentheses was due to this line, which searched for arrow functions that followed the evaluated arrow function within the source code.

This PR removes the calculation of white spaces, and more accurately finds nested arrow functions within the evaluated node.

Is there anything you'd like reviewers to focus on?
Please provide/suggest any other test cases if necessary.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Feb 16, 2019
@ghost ghost changed the title Fix 11268 Fix: implicit-arrow-linebreak adds extra characters (fixes #11268) Feb 16, 2019
Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have a few questions.

tests/lib/rules/implicit-arrow-linebreak.js Outdated Show resolved Hide resolved
lib/rules/implicit-arrow-linebreak.js Outdated Show resolved Hide resolved
lib/rules/implicit-arrow-linebreak.js Outdated Show resolved Hide resolved
@aladdin-add aladdin-add added bug ESLint is working incorrectly rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Feb 18, 2019
@not-an-aardvark
Copy link
Member

Thanks for continuing to work on this!

I'm probably missing something given that the tests are passing, but could you clarify which part of the change prevents the extra closing parentheses from being added? I would have guessed the change happens due to one fewer loop iteration on line 111, but it seems like that only applies to nodes containing nested arrow functions, which isn't the case for the example from #11268.

@ghost
Copy link
Author

ghost commented Mar 11, 2019

@not-an-aardvark This line in the code prevents extra closing parentheses from being added by navigating to the body of the arrow function, and this line prevents the appending of extra parentheses.

@not-an-aardvark
Copy link
Member

Both of those lines are in the addParentheses function. Is the JSDoc comment for that function out of date? The comment says that the function only applies to code containing nested arrow functions, but the example code from #11268 doesn't contain nested arrow functions.

@ghost
Copy link
Author

ghost commented Mar 11, 2019

@not-an-aardvark Thanks for clarifying! Given the example code from #11268

start()
  .then(() => 
    /* If I put a comment here, eslint --fix breaks badly */
    process && typeof process.send === 'function' && process.send('ready')
  )
  .catch(err => {
  	/* catch seems to be needed here */
       console.log('Error: ', err)
  })

when eslint --fix is applied, the output becomes

  /* If I put a comment here, eslint --fix breaks badly */
start()
   .then(() => process && typeof process.send === 'function' && process.send('ready')
    )
   .catch(err => {
    /* catch seems to be needed here */
   console.log('Error: ', err)
    })

And the output for this test case

hello(response =>
  // comment
  response, param => param)

becomes

// comment
hello(response => response, param => param)

Prior to this PR, The JSDoc for addParentheses was incorrect in finding any arrow functions that followed. The above linked line as well this one in autoFixBesides ensure that only nested arrow functions are targeted. Please let me know if any other test cases should be handled.

Copy link
Member

@not-an-aardvark not-an-aardvark 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!

@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implicit-arrow-linebreak autofixer sometimes adds extra characters
4 participants