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

Quote and escape strings with repr #91

Merged
merged 1 commit into from
Dec 8, 2020
Merged

Conversation

mirkobunse
Copy link
Contributor

This PR directly follows from the discussion in #90

@codecov
Copy link

codecov bot commented Oct 15, 2020

Codecov Report

Merging #91 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
- Coverage   80.88%   80.85%   -0.03%     
==========================================
  Files          12       12              
  Lines        1454     1452       -2     
==========================================
- Hits         1176     1174       -2     
  Misses        278      278              
Impacted Files Coverage Δ
src/writer.jl 97.61% <ø> (-0.11%) ⬇️

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 bf30a3d...febbbc4. Read the comment docs.

@christopher-dG
Copy link
Contributor

I think this does fix the issue but it also means that we lose multiline strings, which can be very useful.

On YAML 0.4.2:

julia> println(YAML.write(Dict("foo" => """{\n  "pretty": "json"\n}""")))
foo: |
    {
      "pretty": "json"
    }

Personally I would suggest the following:

a: "plain string"
b: "with a trailing newline, the string is no less readable\n"
c: |
  with
  multiple
  lines
d: |
  with "quotes" inside it
e: |
  with tricky \ backslashes

@mirkobunse
Copy link
Contributor Author

Thanks for your comment!

I agree, actual multi-line strings would be a nice feature. However, I can also imagine that some users actually prefer the single-line fully-escaped solution (or do not care), e.g. when their gut feeling about property c was that a single line is more concise. In the long run, we should provide an argument newlines (or similar) to make the user choose how to handle them. We should then set the default to the behavior you proposed.

My intention behind this PR is to make YAML.write work in the first place. We can handle style in another PR.

@christopher-dG
Copy link
Contributor

I'm happy to make another PR to handle style. Reason being I'm using YAML for saving HTTP response bodies which are often pretty-printed JSON.

@kescobo kescobo self-requested a review December 8, 2020 18:29
@kescobo kescobo added the bug label Dec 8, 2020
@kescobo
Copy link
Collaborator

kescobo commented Dec 8, 2020

Is there any reason anyone can see to not merge this - looks good to me (but I jumped the gun last time...)

@christopher-dG
Copy link
Contributor

I'm pretty sure this should be bulletproof, if not aesthetically pleasing 😄

@kescobo
Copy link
Collaborator

kescobo commented Dec 8, 2020

Good enough for me!

@kescobo kescobo merged commit 01726a8 into JuliaData:master Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants