Fix printing of exact object flow type annotations #1114

Merged
merged 1 commit into from Apr 3, 2017

Conversation

Projects
None yet
2 participants
@lydell
Collaborator

lydell commented Apr 3, 2017

Fixes #1089.

Where do you want me to add a test?

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 3, 2017

Collaborator

@lydell thanks! The location doesn't matter much as long as there is a test for it.

Collaborator

vjeux commented Apr 3, 2017

@lydell thanks! The location doesn't matter much as long as there is a test for it.

@lydell

This comment has been minimized.

Show comment
Hide comment
@lydell

lydell Apr 3, 2017

Collaborator

Test added! While doing so, I noticed that there are no tests at all for exact objects (I ran git grep '{|' tests). Perhaps it is time to sync tests/flow/ with tests/ in the flow repo? I just checked there, and it does contain two {||} so that would have caught this bug :)

Collaborator

lydell commented Apr 3, 2017

Test added! While doing so, I noticed that there are no tests at all for exact objects (I ran git grep '{|' tests). Perhaps it is time to sync tests/flow/ with tests/ in the flow repo? I just checked there, and it does contain two {||} so that would have caught this bug :)

@vjeux vjeux merged commit 96b0a33 into prettier:master Apr 3, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@lydell lydell deleted the lydell:empty-exact-object-type branch Apr 3, 2017

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 3, 2017

Collaborator

I should have cleanly split the flow tests apart from our own, so it should just be a matter of cp -R the flow ones inside of prettier. If you are interested in doing it, that would be awesome :)

Collaborator

vjeux commented Apr 3, 2017

I should have cleanly split the flow tests apart from our own, so it should just be a matter of cp -R the flow ones inside of prettier. If you are interested in doing it, that would be awesome :)

@lydell

This comment has been minimized.

Show comment
Hide comment
@lydell

lydell Apr 3, 2017

Collaborator

I started looking into it, but I ran into a problem and now I'm so confused that I'll have to take a break before continuing :)

Here's what I did: Removed node_modules/ and then ran yarn. The I ran jest (on latest master without any changes). Here the end of its output:

Snapshot Summary
 › 34 snapshot tests failed in 20 test suites. Inspect your code changes or re-run with `-u` to update them.

Test Suites: 21 failed, 436 passed, 457 total
Tests:       34 failed, 1420 passed, 1454 total
Snapshots:   34 failed, 1271 passed, 1305 total
Time:        31.579s
Ran all test suites.

Am I doing something wrong, or are a bunch of snapshots out of date?

Collaborator

lydell commented Apr 3, 2017

I started looking into it, but I ran into a problem and now I'm so confused that I'll have to take a break before continuing :)

Here's what I did: Removed node_modules/ and then ran yarn. The I ran jest (on latest master without any changes). Here the end of its output:

Snapshot Summary
 › 34 snapshot tests failed in 20 test suites. Inspect your code changes or re-run with `-u` to update them.

Test Suites: 21 failed, 436 passed, 457 total
Tests:       34 failed, 1420 passed, 1454 total
Snapshots:   34 failed, 1271 passed, 1305 total
Time:        31.579s
Ran all test suites.

Am I doing something wrong, or are a bunch of snapshots out of date?

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 3, 2017

Collaborator

I have a brand new machine and master works fine for me

git clone https://github.com/prettier/prettier.git
cd prettier/
yarn
yarn test

Test Suites: 457 passed, 457 total
Tests:       1464 passed, 1464 total
Snapshots:   1315 passed, 1315 total
Time:        27.944s
Collaborator

vjeux commented Apr 3, 2017

I have a brand new machine and master works fine for me

git clone https://github.com/prettier/prettier.git
cd prettier/
yarn
yarn test

Test Suites: 457 passed, 457 total
Tests:       1464 passed, 1464 total
Snapshots:   1315 passed, 1315 total
Time:        27.944s
@lydell

This comment has been minimized.

Show comment
Hide comment
@lydell

lydell Apr 3, 2017

Collaborator

Thanks, you're right. Turns out I had accidentally committed some tests without noticing!

When copying over files from the flow repo, I see a lot of deletions in the diffs. Tests seem to have been added to some files since they were copied. My plan is:

  1. Find the additions by us and split them out.
  2. Write a script that syncs with the flow repo, and commit that script.
  3. Run that script and commit the new tests.
  4. Document.

It might take a few days for me to get around doing that. Stay tuned!

Edit: Here’s the PR: #1163

Collaborator

lydell commented Apr 3, 2017

Thanks, you're right. Turns out I had accidentally committed some tests without noticing!

When copying over files from the flow repo, I see a lot of deletions in the diffs. Tests seem to have been added to some files since they were copied. My plan is:

  1. Find the additions by us and split them out.
  2. Write a script that syncs with the flow repo, and commit that script.
  3. Run that script and commit the new tests.
  4. Document.

It might take a few days for me to get around doing that. Stay tuned!

Edit: Here’s the PR: #1163

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 3, 2017

Collaborator

Omg, you're the best!

Collaborator

vjeux commented Apr 3, 2017

Omg, you're the best!

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