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

feat: Use jest-editor-support to show messages etc #23

Merged
merged 3 commits into from
Oct 13, 2017

Conversation

orta
Copy link
Contributor

@orta orta commented Oct 9, 2017

This is going to be a WIP PR, although Danger isn't on this repo, so that might make it hard for me to preview 🗡

@orta
Copy link
Contributor Author

orta commented Oct 9, 2017

This fixes an issue with TSC: jestjs/jest#4636

@orta
Copy link
Contributor Author

orta commented Oct 9, 2017

Found a way to simplify even further

@@ -2,7 +2,7 @@
"compilerOptions": {
"declaration": true,
"declarationDir": "dist",
"lib": ["es5", "es2015"],
"lib": ["es5", "es2015", "es2017"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gives array more functions

src/index.ts Outdated

// console.log(reconciler)

// fails.forEach((failingFile) => fail(fileToFailString(failingFile)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

struggling to get the data the same as vscode-jest - which is surprising, they both do the exact same thing ( run jest, output json, send it into the reconciler)

.map((r: any) => r.failureMessage)
.join('\n')
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a version of this inside jest-editor-support that does more work

@orta
Copy link
Contributor Author

orta commented Oct 9, 2017

OK, this should be good as a rough first draft 👍

Without seeing how Danger JS deals with it, I can't really progress much further.

@@ -0,0 +1,70 @@

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used JSON2TS to stub out the JSON shape

@macklinu
Copy link
Owner

macklinu commented Oct 9, 2017

At first glance, this looks awesome. Appreciate the contribution!

Without seeing how Danger JS deals with it, I can't really progress much further.

I can set up Danger on this repo in another PR/commit and then we can merge it in here to try it out.

@orta
Copy link
Contributor Author

orta commented Oct 9, 2017

Yeah, good call 👍

@macklinu
Copy link
Owner

macklinu commented Oct 9, 2017

#24 has been resolved and merged, so PRs should run Danger now. I tried to check out this branch locally and push a merge commit to your forked master branch, but I'm having trouble figuring out the proper way to do it (or if it's possible?).

@orta
Copy link
Contributor Author

orta commented Oct 9, 2017

In theory you can, I also don't know how to do it - which is why I do branches, updating now

@orta
Copy link
Contributor Author

orta commented Oct 9, 2017

interesting, looks like different versions of Jest test output has different keys available

@orta orta force-pushed the master branch 11 times, most recently from 9df7f01 to 23d0bb5 Compare October 10, 2017 13:04
@orta
Copy link
Contributor Author

orta commented Oct 10, 2017

I'm going to leave this alone for a bit to think, but TBH, I think there's a good argument for letting these tests fail in master also ( so that there's always a visual of what it looks like )

@mxstbr
Copy link

mxstbr commented Oct 10, 2017

I like that output 💯

@orta
Copy link
Contributor Author

orta commented Oct 12, 2017

OK, I'm pretty convinced on this now, 👍

I've skipped the tests which fail.

@macklinu
Copy link
Owner

:shipit:

@macklinu macklinu merged commit 973bcf2 into macklinu:master Oct 13, 2017
@mxstbr
Copy link

mxstbr commented Oct 13, 2017

New release?! 💯

@macklinu
Copy link
Owner

Yep! Give 1.1.0 a try and leave any feedback/bug reports in a new issue. ❤️

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.

3 participants