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

add hyperprint to test diffs #1902

Closed
wants to merge 1 commit into from
Closed

Conversation

ianstormtaylor
Copy link
Owner

Is this adding or improving a feature or fixing a bug?

Improvement.

What's the new behavior?

Uses slate-hyperprint to make the diff for test failures easier to read.

Have you checked that...?

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn prettier.)
  • The relevant examples still work. (Run examples with yarn watch.)

Does this fix any issues or need any specific reviewers?

Waiting on GitbookIO/slate-hyperprint#3 for selection support, since a bunch of the tests need selections to be serialized.

@zarv1k
Copy link
Contributor

zarv1k commented Oct 31, 2018

Hey @ianstormtaylor, I've already implemented selection+decorations support for slate@^0.40. But seems my PR won't be merged, because you know that guys from Gitbook forked Slate at ^0.34 so they aren't going to update to ^0.40 at all.

Anyway hyperscript+hyperprint would be really useful for tests. I'm using it in my project and found it very useful for dumping Slate Values in hyperscript jsx string and immediately use it in my new test. Diffing in jsx format in tests is also much more understandable than in JSON.

I think it would be great if the fork of hyperprint package is supported by Slate community with the latest hyperscript features. What do you think?

Thank you.

@ianstormtaylor
Copy link
Owner Author

Hey @zarv1k, I would love to have it as part of core actually. I was thinking about it, and I think it might make sense to have the hyperscript package expose the printer as well. That way you can do:

const h = createHyperScript({ ... })`
const string = h.print(...)

And both would use the same set of shorthands. What do you think? Would you be down to add a PR that adds it to core? (I realize it would be a lot, because it would require removing the Flow logic from that other PR.)

Side note, do you have a screenshot of what the test diffs look like? I'm just curious, because it sounds really nice.

@zarv1k
Copy link
Contributor

zarv1k commented Oct 31, 2018

@ianstormtaylor, as you requested, I've broken one of my 'changes test' of list and fail result using 'slate-hyperprint' looks like that.
Is it suitable example?

Good to hear that you would love to have it as part of core. I don't mind to help with that. As you noticed correctly it's going to be a lot of work and seems that the removal of Flow is not the biggest problem :) E.g. at first, slate-hyperprint should be updated for using slate@^0.43 as dependency. Anyway I'll try to find time to do that.

I'm just not sure at the moment how exactly should I do that in details (I mean moving the whole codebase of slate-hyperprint into the repo) and perhaps I'll need some help, so I'll prepare some checklist soon for discussion. Shall we communicate using Slack to discuss that?

@ianstormtaylor
Copy link
Owner Author

@zarv1k That example looks great, it would be very cool to have that functionality. As for communicating, Slack sounds great! Or in here, either way.

@ianstormtaylor
Copy link
Owner Author

Now that we have JSON support, I think this is not necessary anymore.

@zarv1k
Copy link
Contributor

zarv1k commented Nov 29, 2019

@ianstormtaylor , just FYI, there is my fork of hyperprint, which is compatible with 0.47.x - @zarv1k/slate-hyperprint

@ianstormtaylor ianstormtaylor deleted the add-hyperprint-tests branch December 10, 2019 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants