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

fix printing of tables inside arrays and https://github.com/JuliaLang/julia/issues/46454 #40

Merged
merged 1 commit into from
Oct 7, 2022

Conversation

KristofferC
Copy link
Member

@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Merging #40 (85912cc) into master (9cf4962) will decrease coverage by 0.23%.
The diff coverage is 89.74%.

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
- Coverage   93.54%   93.30%   -0.24%     
==========================================
  Files           3        3              
  Lines         697      702       +5     
==========================================
+ Hits          652      655       +3     
- Misses         45       47       +2     
Impacted Files Coverage Δ
src/print.jl 94.01% <89.74%> (-1.52%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@tecosaur
Copy link

tecosaur commented Oct 7, 2022

I've just tested this, and can confirm that in my previously-broken use case, it now works.

One minor comment, that's more style than anything else, might it be possible to use inline Dicts for short expressions? At the moment the eager expansion into non-inline dicts can make the TOML output a bit noiser than need be.

For example, unprocessed:

[["GTEx v8 transcript tpm"]]
stuff="..."
    [["GTEx v8 transcript tpm".loader]]
    stuff="..."
        [["GTEx v8 transcript tpm".loader.loaders]]
        driver = "csv"
        args = { header = 3 }

becomes (via a parsing round-trip)

[["GTEx v8 transcript tpm"]]
stuff="..."
    [["GTEx v8 transcript tpm".loader]]
    stuff="..."
        [["GTEx v8 transcript tpm".loader.loaders]]
        driver = "csv"
        
            ["GTEx v8 transcript tpm".loader.loaders.args]
            header = 3

which I think is quite simply harder to read.

Would it take much to add a heuristic along the lines of "if number of key-value pairs less than (some N) and inline dict representation is <72 characters long, use an inline dict" ?

@KristofferC
Copy link
Member Author

KristofferC commented Oct 7, 2022

Yeah, there are some heuristics we could probably improve here. It would be good to see what other TOML exporters do. Do they have some API where you specify how things should look or do they just go via heuristics etc? Some research here could help us decide how to best move forward.

In the meantime, I will merge this since it fixes two issues.

@KristofferC KristofferC merged commit cac206d into master Oct 7, 2022
@tecosaur
Copy link

tecosaur commented Oct 7, 2022

Glad to see this merged! Thanks for fixing the issue 🙂 I'm guessing this will make its way into patch versions of Julia? Or will I need to require Julia 1.9+ in my package?

Lastly, re: style, seems like I should probably make a new issue for that.

@tecosaur
Copy link

If someone might answer my question in the comment above on which versions of Julia this fix will be found in, that would be nice 🙂.

@KristofferC
Copy link
Member Author

Will be in 1.9. Might be in 1.8.4.

@tecosaur
Copy link

Thanks, that's helpful to know 👍

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.

TOML.print produces invalid/unparsable output TOML: using to_toml option may produce incorrect TOML
2 participants