Skip to content

Conversation

t-bltg
Copy link
Contributor

@t-bltg t-bltg commented Aug 10, 2022

Fix #116, all tests pass locally.

Needs JuliaImages/ImageInTerminal.jl#71 and JuliaImages/XTermColors.jl#8.

Ping @johnnychen94 for review, since I cannot request for a review here (not a member of JuliaTesting org).

@t-bltg t-bltg marked this pull request as draft August 10, 2022 11:41
@t-bltg t-bltg marked this pull request as ready for review August 10, 2022 11:49
@oxinabox oxinabox requested a review from johnnychen94 August 10, 2022 17:45
@oxinabox
Copy link
Member

This PR would be much better if it was broken into two.
One which supports ImageInTerminals v0.5 and updates all reference images
and one containing all other changes.

@t-bltg
Copy link
Contributor Author

t-bltg commented Aug 10, 2022

This PR would be much better if it was broken into two.

Agreed, will split (EDIT: see #118).

@johnnychen94
Copy link
Member

Maybe we'd just switch to XTermColors.ascii_show? For test utils like ReferenceTests, it actually doesn't make much sense to override the default display or Base.show methods -- it might have unexpected side-effects. For instance, if people use Documenter.doctest in test/runtests.jl, it will be ruined.

@t-bltg
Copy link
Contributor Author

t-bltg commented Aug 11, 2022

Maybe we'd just switch to XTermColors.ascii_show

This is very clever and benefits from the rework / splitting of ImageInTerminal 0.5 (credits to @johnnychen94 for the big picture):

  1. it avoids ImageInTerminal side effects e.g. overriding Base.show;
  2. it reduces dependencies, thus improving load times;
  3. it solves the circular dependency at the origin of ReferenceTests is not compatible with ImageInTerminal 0.5.0 #116.

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Looks like we'd need to release JuliaImages/XTermColors.jl#8 (review) first

@t-bltg t-bltg changed the title support ImageInTerminal 0.5 drop ImageInTerminal Aug 11, 2022
@johnnychen94
Copy link
Member

@t-bltg let me know when you're ready for ReferenceTests v0.10 release.

@johnnychen94 johnnychen94 merged commit d747ed8 into JuliaTesting:master Aug 11, 2022
@t-bltg t-bltg deleted the imt branch August 11, 2022 13:26
@t-bltg
Copy link
Contributor Author

t-bltg commented Aug 11, 2022

When you want, so that we can go forward on JuliaImages/ImageInTerminal.jl#71.

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.

ReferenceTests is not compatible with ImageInTerminal 0.5.0
3 participants