Skip to content

Conversation

@mirkobunse
Copy link
Contributor

These are two minor changes which are combined in a single PR because they can nicely be tested together.

Writing AbstractDicts

This feature solves #82. As pointed out in this issue, writing instances of AbstractDict allows users to make use of ordered dictionaries like OrderedCollections.OrderedDict. Thus, the user can determine in which order the properties are written.

Note that none of the tests is broken (i.e. all test instances are written correctly) if the Dict requirement is dropped in favor of AbstractDict. Hurray! I conclude that my initial implementation of writing YAML Strings (#68) was unnecessarily picky with the Dict type.

YAML.yaml

YAML.yaml returns a YAML-formatted String representation of a dictionary. It is implemented as a simple alias for YAML.write, which could have done the same thing already. The name YAML.yaml, however, complies with the beautiful API of JSON.jl, which has JSON.json return a JSON-formatted String representation of a dictionary.

This feature takes a step towards #70 without making any breaking change (only an alias is provided). Also in #29, it has been argued in favor of a YAML.yaml method: #29 (comment)

…urns a String.

Writing AbstractDicts has the advantage that OrderedDicts become available for writing properties in a fixed order. YAML.yaml is an alias for YAML.write, which complies with the API of JSON.jl
@codecov
Copy link

codecov bot commented Aug 12, 2020

Codecov Report

Merging #86 into master will increase coverage by 0.26%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #86      +/-   ##
==========================================
+ Coverage   74.30%   74.56%   +0.26%     
==========================================
  Files          12       12              
  Lines        1331     1333       +2     
==========================================
+ Hits          989      994       +5     
+ Misses        342      339       -3     
Impacted Files Coverage Δ
src/writer.jl 95.12% <100.00%> (+14.16%) ⬆️
src/composer.jl 89.74% <0.00%> (-1.17%) ⬇️
src/constructor.jl 70.00% <0.00%> (-0.47%) ⬇️
src/parser.jl 70.55% <0.00%> (-0.23%) ⬇️

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 eb7af07...9b24a75. Read the comment docs.

@kescobo
Copy link
Member

kescobo commented Aug 13, 2020

This all makes sense to me, but I'm not a big user of this package. I'll merge in a couple of days if there are no objections so that it doesn't languish, but can you maybe post on slack or zulip or discourse to see if there's any feedback from people that are actually using it?

@mirkobunse
Copy link
Contributor Author

What exactly should I ask?

My motivation to open this PR was mostly the last point I mentioned: taking a step towards #70. What I did not yet mention is that, besides the trivial YAML.yaml, also the writing of OrderedDict instances is preparing a solution to #70. Why?

JSON.jl allows the user to pick the subtype of AbtractDict that should be produced by the parser. If a user picks an OrderedDict, it will reflect the order of properties in the JSON file. Since writing supports OrderedDict instances, this order is maintained further. Consequently, a round-trip read/write yields highly similar files.

Some users of YAML.jl have discussed this behaviour as a desireable feature since quite some time, in #29. With writing instances of AbstractDict, they are one step closer to this feature.

The PR we are discussing here is nothing fancy, for sure, but it'll help get some other things done.

@kescobo
Copy link
Member

kescobo commented Aug 15, 2020

What exactly should I ask?

Basically just - "can people that use YAML.jl take a look at this PR and provide feedback or raise concerns if you have them?"

I'm sure it's fine, especially if you expect it to be non-breaking - I just don't have any experience with this package, and I'm worried that the people who do aren't watching the issues / PRs.

@mirkobunse
Copy link
Contributor Author

@GunnarFarneback
Copy link
Contributor

I have no opinion either way about the YAML.yaml function. The rest of the PR is uncontroversial and fine.

@mirkobunse
Copy link
Contributor Author

@kescobo, how long do you plan to wait before merging?

I want to continue with #70 (comment) but not before this PR is done

@kescobo kescobo merged commit a4f967d into JuliaData:master Aug 20, 2020
@kescobo
Copy link
Member

kescobo commented Aug 20, 2020

All set 👍.

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.

3 participants