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

Round Trip Printing. update tests for repr and show methods #470

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

anandijain
Copy link

also adds a test so that the output printed to stdout can be copy pasted (roundtripped) back into REPL

supercedes #467

@anandijain
Copy link
Author

@sostock i would appreciate if you could have a peek at this when you get a chance

test/runtests.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
test/runtests.jl Show resolved Hide resolved
src/display.jl Outdated Show resolved Hide resolved
src/display.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
anandijain and others added 6 commits July 26, 2021 14:50
Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>
Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>
Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>
Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>
Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>
Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>
@anandijain
Copy link
Author

thanks a bunch!

@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2021

Codecov Report

Merging #470 (86bf322) into master (492c475) will decrease coverage by 0.79%.
The diff coverage is 42.10%.

❗ Current head 86bf322 differs from pull request most recent head 9caac72. Consider uploading reports for the commit 9caac72 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #470      +/-   ##
==========================================
- Coverage   83.77%   82.98%   -0.80%     
==========================================
  Files          16       16              
  Lines        1344     1340       -4     
==========================================
- Hits         1126     1112      -14     
- Misses        218      228      +10     
Impacted Files Coverage Δ
src/logarithm.jl 69.54% <0.00%> (ø)
src/types.jl 89.79% <0.00%> (ø)
src/utils.jl 93.02% <0.00%> (-2.33%) ⬇️
src/display.jl 84.70% <57.14%> (-10.54%) ⬇️
src/user.jl 94.07% <0.00%> (-0.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 492c475...9caac72. Read the comment docs.

else
@test string(typeof(1.0m/s)) ==
"Quantity{Float64,𝐋 𝐓^-1,FreeUnits{(m, s^-1),𝐋 𝐓^-1,nothing}}"
"Quantity{Float64, Unitful.Dimensions{(Unitful.Dimension{:Length}(1//1), Unitful.Dimension{:Time}(-1//1))}(), FreeUnits{(Unitful.Unit{:Meter, Unitful.Dimensions{(Unitful.Dimension{:Length}(1//1),)}()}(0, 1//1), Unitful.Unit{:Second, Unitful.Dimensions{(Unitful.Dimension{:Time}(1//1),)}()}(0, -1//1)), Unitful.Dimensions{(Unitful.Dimension{:Length}(1//1), Unitful.Dimension{:Time}(-1//1))}(), nothing}}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This (and some other tests) would need to be changed to match the old printing of type parameters (no spaces after commas), that’s why that @static if VERSION ≥ v"1.6.0-DEV.770" exists. But IMO we could just delete these tests since the printing isn’t customized anymore.

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.

4 participants