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

Use munit diff logic #25

Merged
merged 10 commits into from Apr 26, 2024
Merged

Use munit diff logic #25

merged 10 commits into from Apr 26, 2024

Conversation

zainab-ali
Copy link
Contributor

@zainab-ali zainab-ali commented Apr 16, 2024

As part of #15 , we wish to improve the diff rendering of the Comparison trait by using a Myers diff implementation. munit's Myers diff algorithm is a prime candidate.

As discussed on Discord, we prefer to vendor the diff logic than pull in an external dependency.

This PR is an experiment in vendoring.

Note that:

  • tests must be updated.
  • the scala-diff library is yet to be published.

@zainab-ali zainab-ali marked this pull request as draft April 16, 2024 12:22
@zainab-ali
Copy link
Contributor Author

To summarize the discussions on this, there are two approaches we could take to improving the diff logic:

  • Vendor the munit diff code, as is done in the current changeset.
  • Pull in the munit-diff library dependency.

I'm exploring pulling in the library dependency, but shading it such that users of weaver and munit are unaffected. Given that the lib is unlikely to change, but is on the same release cadence as munit, releases will also be excluded from Scala Steward.

@Baccata
Copy link
Collaborator

Baccata commented Apr 23, 2024

Changes look good 👍

Do you have a rough idea of when munit-diff could be released ?

@zainab-ali
Copy link
Contributor Author

zainab-ali commented Apr 23, 2024

Thanks! The PR has no obvious outstanding issues, so I expect it to be merged in the next few days. I'm not sure of the munit release cycle - @tgodzik may be able to comment.

@tgodzik
Copy link

tgodzik commented Apr 23, 2024

Thanks! The PR has no obvious outstanding issues, so I expect it to be merged in the next few days. I'm not sure of the munit release cycle - @tgodzik may be able to comment.

We plan to do munit 1.0.0-RC1 after that is merged and if nothing pops up later on, we'll release a full 1.0.0

@tgodzik
Copy link

tgodzik commented Apr 25, 2024

Munit and munit-diff 1.0.0-RC1 is out. I also released manually for 0.4 Scala Native. Do let me know if it works!

@zainab-ali zainab-ali marked this pull request as ready for review April 25, 2024 12:05
@Baccata
Copy link
Collaborator

Baccata commented Apr 26, 2024

Merging even if it depends on an RC, because it's shaded anyway, and because there's still a little bit of work to be done before an actual release of weaver, which may give munit enough time for a 1.0.0 release

@Baccata Baccata merged commit f9b5683 into typelevel:main Apr 26, 2024
13 checks passed
@zainab-ali zainab-ali deleted the diff branch April 26, 2024 08:38
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.

None yet

3 participants