Fix comment sorting location #1038

Merged
merged 1 commit into from Mar 20, 2017

Conversation

Projects
None yet
2 participants
@vjeux
Collaborator

vjeux commented Mar 19, 2017

Right now it's only doing one test for begin/end, but in the issue #1037, we have two nodes that have the same start but different end. The current implementation incorrectly sorts them and the identifier ends up being before the container and therefore the comment search doesn't recurse into it.

Fixes #1037
Fixes #938

Fix comment sorting location
Right now it's only doing one test for begin/end, but in the issue #1037, we have two nodes that have the same start but different end. The current implementation incorrectly sorts them and the identifier ends up being before the container and therefore the comment search doesn't recurse into it.

Fixes #1037
@@ -13,8 +13,7 @@ const { a /* comment */ = 1 } = b;
const { c = 1 /* comment */ } = d;
let {
- //comment
- a = b
+ a = b //comment

This comment has been minimized.

@vjeux

vjeux Mar 19, 2017

Collaborator

It was

let {a //comment
= b} = c

so both before and after are okay.

@vjeux

vjeux Mar 19, 2017

Collaborator

It was

let {a //comment
= b} = c

so both before and after are okay.

declare module bar {
+ // TODO

This comment has been minimized.

@vjeux

vjeux Mar 19, 2017

Collaborator

The input is

// This file triggers a violation of the "disjoint-or-nested ranges invariant"
// that we implicitly assume in type-at-pos and coverage implementations. In
// particular, when unchecked it causes a crash with coverage --color.

declare module foo {
}

declare module bar {
}

// TODO

so while not perfect, the new behavior is much better.

@vjeux

vjeux Mar 19, 2017

Collaborator

The input is

// This file triggers a violation of the "disjoint-or-nested ranges invariant"
// that we implicitly assume in type-at-pos and coverage implementations. In
// particular, when unchecked it causes a crash with coverage --color.

declare module foo {
}

declare module bar {
}

// TODO

so while not perfect, the new behavior is much better.

-}
-/**
+
+ /**

This comment has been minimized.

@vjeux

vjeux Mar 19, 2017

Collaborator

The flow parser has completely busted parsing location for this one. It happened to do the right thing in the previous version by chance.

screen shot 2017-03-18 at 5 59 27 pm

@vjeux

vjeux Mar 19, 2017

Collaborator

The flow parser has completely busted parsing location for this one. It happened to do the right thing in the previous version by chance.

screen shot 2017-03-18 at 5 59 27 pm

This comment has been minimized.

@jlongster

jlongster Mar 20, 2017

Member

Haha, have you filed a bug with Flow?

@jlongster

jlongster Mar 20, 2017

Member

Haha, have you filed a bug with Flow?

This comment has been minimized.

@vjeux

vjeux Mar 20, 2017

Collaborator

I did, internally.

@vjeux

vjeux Mar 20, 2017

Collaborator

I did, internally.

+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+type Foo = {
+ method(
+ arg: number, // I belong with baz

This comment has been minimized.

@vjeux

vjeux Mar 19, 2017

Collaborator

now it doesn't inline the method anymore!

@vjeux

vjeux Mar 19, 2017

Collaborator

now it doesn't inline the method anymore!

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Mar 20, 2017

Member

Very nice catch! I'll approve and let you merge in case you want to do anything else

Member

jlongster commented Mar 20, 2017

Very nice catch! I'll approve and let you merge in case you want to do anything else

@vjeux vjeux merged commit 5539663 into prettier:master Mar 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment