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

Added ability to work with --fix #11

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Sawtaytoes
Copy link

@Sawtaytoes Sawtaytoes commented Feb 28, 2020

I added the missing --fix functionality.

Using --fix requires indent from ESLint for it to make sure indentation is correct after fixing.

Apparently the other newline fixes in ESLint have the same issue of requiring indent to properly reformat on fix.

This requires `indent` from ESLint for it to make sure indentation is correct after fixing. Apparently the other newline fixes in ESLint have the same issue of requiring `indent` to properly reformat on fix.
@Sawtaytoes Sawtaytoes mentioned this pull request Feb 28, 2020
@getify
Copy link
Owner

getify commented Feb 28, 2020

14 of the PARENS tests are failing, as indicated here:

https://travis-ci.org/getify/eslint-plugin-proper-ternary/jobs/656153042

Can you check to see why?

@Sawtaytoes
Copy link
Author

Sawtaytoes commented Feb 28, 2020

@getify Took me a while, but I figured it out. No fix yet.

This is adding parens only to sub-expressions in ternaries:

"ConditionalExpression": function enter(node) {
  for (let clause of [node.test,node.consequent,node.alternate,]) {
    let exprType =
      (clause.type == "ConditionalExpression") ? "ternary" :
      (clause.type == "BinaryExpression" && ["==","===","!=","!==","<",">","<=",">=","in","instanceof",].includes(clause.operator)) ? "comparison" :
      (clause.type == "LogicalExpression") ? "logical" :
      (clause.type == "UnaryExpression" && clause.operator == "!") ? "logical" :
      (["CallExpression","NewExpression",].includes(clause.type)) ? "call" :
      (["ArrayExpression","ObjectExpression",].includes(clause.type)) ? "object" :
      (["Identifier","MemberExpression","Literal","TemplateLiteral",].includes(clause.type)) ? "simple" :
      "complex";

For whatever reason, my fixes have an assumption these sub-expressions are only ConditionalExpression; therefore, it sometimes tries to grab sourceCode.getTokenBefore(clause.test) which only works if this is a ternary.

To pass all tests, this could be modified like so:

const tokenBefore = sourceCode.getTokenBefore(clause)
const tokenAfter = sourceCode.getTokenAfter(clause)

@Sawtaytoes
Copy link
Author

Sawtaytoes commented Feb 28, 2020

Yep, that's the fix; although, my changes add newlines even to single-line statements. That'll also need to be addressed before a merge.

Lastly, there's an issue when blocks (array, functions, and objects) are wrapped. I'll be adding that functionality as well.

@Sawtaytoes
Copy link
Author

Sawtaytoes commented Feb 28, 2020

I added the ability to keep it single-line with addParensOnSameLine and added another one (func) allowing you to disable parens for nested functions.

This is ready for merge :); although, it's missing tests and documentation :/.

@getify
Copy link
Owner

getify commented Apr 13, 2020

Thanks for getting the existing tests to pass.

Definitely want to get the new tests in, so we can validate that it's doing what's expected in all the various cases. After that, we can document it.

If you're able to add new tests, that's appreciated. I haven't been able to circle back to this and work on it, regrettably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants