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.

Sign up for GitHub

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

Jump to bottom

feat(printer): mk printers more easily configurable #640

Merged
merged 8 commits into from
Apr 5, 2023
Merged

feat(printer): mk printers more easily configurable #640

merged 8 commits into from
Apr 5, 2023

Conversation

wahtique
Copy link
Contributor

@wahtique wahtique commented Mar 27, 2023 •
edited
Loading

Hi !

Following this issue I opened #637 and closed right after once I realised I should havve looked around a bit more, I tried my hand at using custom pretty printers to clarify some of my diffs.

Using the answer to this question #480 I got pretty much what a I needed ( comparing scodecs ByteVectors ) but I found the experience a bit confusing and felt some boilerplate could be avoided.

Which leads me to this PR :)

Changes :

  • add orElse combinator for Printer for composability
  • add utility constructor for easier instantiation : user will only pass whatever is actually meaningful to them in most cases, ie. the custom logic to turn their test values into differenciable strings
  • update Assertion to allow users to pass their custom Printer without also overriding munitPrint

@wahtique
feat(printer): mk printers more easily configurable
4e3cfd8 
- add `orElse` combinator for `Printer` for composability
- add utility constructor for easier instantiation : user will only pass whatever is actually meaningful to them in most cases, ie. the custom  logic to stringify the test values
- update `Assertion` to allow users to pass their custom `Printer` without overriding `munitPrint`
@wahtique
Copy link
Contributor Author

Comment taken into account. I also restricted some tests to scala 2.13 like in munit.PrintersSuite and fixed the Printer implem to compile under 2.12

@tgodzik
Copy link
Contributor

tgodzik commented Mar 29, 2023

Looks like the tests are passing! Let's see if anyone more familiar with the codebase has time to look at it.

@tgodzik tgodzik requested review from gabro and olafurpg March 29, 2023 16:37
gabro
gabro requested changes Mar 30, 2023
View reviewed changes
Copy link
Member

@gabro gabro left a comment

Choose a reason for hiding this comment

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

I think the changes make sense; it seems like a reasonable API to add.

I've left some code-style comments inline.

I also think adding some documentation on how to customize the printer would be good. It could be a separate section on the website under Overview

munit/shared/src/main/scala/munit/Printer.scala Outdated Show resolved Hide resolved
munit/shared/src/main/scala/munit/Printer.scala Outdated Show resolved Hide resolved
@wahtique
Copy link
Contributor Author

wahtique commented Apr 1, 2023

Comments taken into account in the latest commits. Also added section covering the why and how of custom printers in https://scalameta.org/munit/docs/tests.html right under custom timeouts. Please tell me if its clear enough and / or if you would prefer it elsewhere :)

@wahtique wahtique requested a review from gabro April 2, 2023 12:06
tgodzik
tgodzik requested changes Apr 5, 2023
View reviewed changes
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Could you run scalafix and scalafmt ? Otherwise looks good, unless @gabro sees anything more to fix?

munit/shared/src/main/scala/munit/Printer.scala Outdated Show resolved Hide resolved
gabro
gabro approved these changes Apr 5, 2023
View reviewed changes
Copy link
Member

@gabro gabro left a comment

Choose a reason for hiding this comment

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

Looking good to me :)

@wahtique
Copy link
Contributor Author

wahtique commented Apr 5, 2023

scalafmt and scalafix ran without apparent issue

But I've got some errors from doc/docusaurusCreateSite which seem unrelated to this PR
image

Any idea why ?

@wahtique wahtique requested a review from tgodzik April 5, 2023 09:37
tgodzik
tgodzik approved these changes Apr 5, 2023
View reviewed changes
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

@tgodzik
Copy link
Contributor

tgodzik commented Apr 5, 2023

scalafmt and scalafix ran without apparent issue

But I've got some errors from doc/docusaurusCreateSite which seem unrelated to this PR image

Any idea why ?

That might be an existing issue, I don't think your change could have influenced anything

@tgodzik tgodzik merged commit 22a27a7 into scalameta:main Apr 5, 2023
@alexgb1
Copy link

alexgb1 commented Jun 1, 2023

Any plans to release this change soon ?

@valencik valencik mentioned this pull request Jun 9, 2023
Closed
mzuehlke added a commit to mzuehlke/circe that referenced this pull request Jul 17, 2023
@mzuehlke
munit 1.0.0-M8 with test adaptation
28e7dff 
scalameta/munit#640 requires to change the name of the `io.circe.Printer` val.
@mzuehlke mzuehlke mentioned this pull request Jul 17, 2023
Merged
@mzuehlke mzuehlke mentioned this pull request Apr 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgodzik tgodzik tgodzik approved these changes

@gabro gabro gabro approved these changes

@olafurpg olafurpg Awaiting requested review from olafurpg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wahtique @tgodzik @alexgb1 @gabro