Fix additional empty line switch case comment #936

Merged
merged 8 commits into from Mar 17, 2017

Conversation

Projects
None yet
2 participants
@yamafaktory
Contributor

yamafaktory commented Mar 7, 2017

This one should fix #805.

yamafaktory added some commits Mar 7, 2017

src/printer.js
@@ -1855,7 +1856,17 @@ function printStatementSequence(path, options, print) {
parts.push(stmtPrinted);
- if (util.isNextLineEmpty(text, stmt) && !isLastStatement(stmtPath)) {
+ if (parent.type === "SwitchCase") {

This comment has been minimized.

@vjeux

vjeux Mar 8, 2017

Collaborator

This function is a generic one. Could you instead try to fix it inside of the implementation of SwitchCase?

@vjeux

vjeux Mar 8, 2017

Collaborator

This function is a generic one. Could you instead try to fix it inside of the implementation of SwitchCase?

This comment has been minimized.

@yamafaktory

yamafaktory Mar 8, 2017

Contributor

Of course, I'll do it.

@yamafaktory

yamafaktory Mar 8, 2017

Contributor

Of course, I'll do it.

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 9, 2017

Contributor

@vjeux does it match your expectations? Thanks!

Contributor

yamafaktory commented Mar 9, 2017

@vjeux does it match your expectations? Thanks!

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 16, 2017

Contributor

@vjeux Can you check again that one, thanks!

Contributor

yamafaktory commented Mar 16, 2017

@vjeux Can you check again that one, thanks!

src/printer.js
@@ -1834,8 +1841,9 @@ function genericPrintNoParens(path, options, print) {
function printStatementSequence(path, options, print) {
let printed = [];
- path.map(function(stmtPath, i) {
- var stmt = stmtPath.getValue();
+ path.map(stmtPath => {

This comment has been minimized.

@vjeux

vjeux Mar 16, 2017

Collaborator

can you revert those 3 lines, they are unrelated.

@vjeux

vjeux Mar 16, 2017

Collaborator

can you revert those 3 lines, they are unrelated.

This comment has been minimized.

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Oops, correct!

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Oops, correct!

This comment has been minimized.

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Actually the i is unused.

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Actually the i is unused.

This comment has been minimized.

@vjeux

vjeux Mar 16, 2017

Collaborator

You should send a separate pull request to remove the i, not add it to a random one :)

@vjeux

vjeux Mar 16, 2017

Collaborator

You should send a separate pull request to remove the i, not add it to a random one :)

This comment has been minimized.

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

No pbl, thanks will do it!

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

No pbl, thanks will do it!

src/printer.js
+ let printed = [];
+ path.map(p => {
+ const value = p.getValue();
+ const parent = p.getParentNode();

This comment has been minimized.

@vjeux

vjeux Mar 16, 2017

Collaborator

can you avoid creating those two variables, you are only using them once and it makes it more verbose than needs be

@vjeux

vjeux Mar 16, 2017

Collaborator

can you avoid creating those two variables, you are only using them once and it makes it more verbose than needs be

This comment has been minimized.

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Yeah, you're right, was doing that for readability only.

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Yeah, you're right, was doing that for readability only.

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 16, 2017

Contributor

@vjeux done!

Contributor

yamafaktory commented Mar 16, 2017

@vjeux done!

@yamafaktory yamafaktory referenced this pull request Mar 16, 2017

Merged

Remove unused code. #1028

@vjeux

Okay, I now understand what is going on. The empty line is both matched by the comment and the inner expression inside of the switch case. So what you are doing is to remove the one from the switch case, which seems to be the correct decision since there's usually a closing character that would prevent this from happening like:

switch(1) {
  case 1: {
    doSomething();
  }

  // comment
}

Let's fix the few comments I made and we can ship it. It would be nice for the next pull requests that are a bit complicated like this one to explain what is going on and why you decided to fix it the way you did. It makes it a lot easier to review code ;)

src/printer.js
+ const cons = path.call(consequentPath => {
+ let printed = [];
+ path.map(p => {
+ const parentParent = path.getParentNode(1);

This comment has been minimized.

@vjeux

vjeux Mar 16, 2017

Collaborator

parentParent and last do not change on every iteration, you can put them outside of the loop

@vjeux

vjeux Mar 16, 2017

Collaborator

parentParent and last do not change on every iteration, you can put them outside of the loop

This comment has been minimized.

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Oh you're right!

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Oh you're right!

src/printer.js
+ path.map(p => {
+ const parentParent = path.getParentNode(1);
+ const last = parentParent && parentParent.cases &&
+ parentParent.cases[parentParent.cases.length - 1];

This comment has been minimized.

@vjeux

vjeux Mar 16, 2017

Collaborator

use util.getLast()

@vjeux

vjeux Mar 16, 2017

Collaborator

use util.getLast()

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 16, 2017

Contributor

@vjeux partially done. Strangely moving parentParent and last out of the loop breaks it...

Contributor

yamafaktory commented Mar 16, 2017

@vjeux partially done. Strangely moving parentParent and last out of the loop breaks it...

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 16, 2017

Contributor

@vjeux Forget about what I said before, fixed now!

Contributor

yamafaktory commented Mar 16, 2017

@vjeux Forget about what I said before, fixed now!

src/printer.js
+ const lastCase = util.getLast(parent.cases);
+ const cons = path.call(consequentPath => {
+ let printed = [];
+ path.map(p => {

This comment has been minimized.

@vjeux

vjeux Mar 16, 2017

Collaborator

I just realized that map creates an array with the returned values. So you can do

return join(hardline, path.map(p => {
  ...
  return concat(...);
});

to make the code shorter.

Sorry my feedback is coming in waves. This is the only last thing I'm seeing with your code!

@vjeux

vjeux Mar 16, 2017

Collaborator

I just realized that map creates an array with the returned values. So you can do

return join(hardline, path.map(p => {
  ...
  return concat(...);
});

to make the code shorter.

Sorry my feedback is coming in waves. This is the only last thing I'm seeing with your code!

This comment has been minimized.

@vjeux

vjeux Mar 16, 2017

Collaborator

Shouldn't this be consequentPath?

@vjeux

vjeux Mar 16, 2017

Collaborator

Shouldn't this be consequentPath?

This comment has been minimized.

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

You mean instead of p?

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

You mean instead of p?

src/printer.js
+ const parent = path.getParentNode();
+ const lastCase = util.getLast(parent.cases);
+ const cons = path.call(consequentPath => {
+ return join(hardline, path.map(p => {

This comment has been minimized.

@vjeux

vjeux Mar 16, 2017

Collaborator

You are doing path.call in order to get consequentPath but you're not using it. So either the path.call is useless and you should remove it, or there's a problem with the code and path.map should be consequentPath.map and it happens to be working because path is mutated under the scenes.

@vjeux

vjeux Mar 16, 2017

Collaborator

You are doing path.call in order to get consequentPath but you're not using it. So either the path.call is useless and you should remove it, or there's a problem with the code and path.map should be consequentPath.map and it happens to be working because path is mutated under the scenes.

This comment has been minimized.

@yamafaktory

yamafaktory Mar 17, 2017

Contributor

That was definitely the second solution. I need consequentPath in this case. Sorry for the confusion, I refactored it so many times before that I didn't see that one 👍.

@yamafaktory

yamafaktory Mar 17, 2017

Contributor

That was definitely the second solution. I need consequentPath in this case. Sorry for the confusion, I refactored it so many times before that I didn't see that one 👍.

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 17, 2017

Contributor

@vjeux should be good to go now 🤞.

Contributor

yamafaktory commented Mar 17, 2017

@vjeux should be good to go now 🤞.

@vjeux vjeux merged commit dde8463 into prettier:master Mar 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 17, 2017

Collaborator

Yay!

Collaborator

vjeux commented Mar 17, 2017

Yay!

@yamafaktory yamafaktory deleted the yamafaktory:805-fix-additional-empty-line-switch-case-comment branch Mar 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment