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

Support serialization to "pretty" output #11

Merged
merged 1 commit into from
Oct 2, 2020
Merged

Support serialization to "pretty" output #11

merged 1 commit into from
Oct 2, 2020

Conversation

homeworkprod
Copy link
Contributor

@homeworkprod homeworkprod commented Oct 2, 2020

Hi Samuel, thanks for the nice library!

I found myself interested in "pretty" output because line-wrapped arrays make for more compact diffs of TOML under version control.

In the suggested implementation I have opted for a keyword argument in the Python layer to enable the new behavior on demand, but went with a separate function (unfortunately with a bit of code duplication) in Rust due to its lack of optional, default, and/or named arguments (as of mid-2020).

Note regarding the test cases: According to the documentation of toml::ser::Serializer::pretty_string, the "pretty" output for text = "\nfoo\nbar\n" should be

text = '''
foo
bar
'''

but I found it to be this instead (notice the blank line after the opening triple quotes):

text = '''

foo
bar
'''

Please let me know what you think and have a nice weekend!

@homeworkprod
Copy link
Contributor Author

Unfortunately, the CI build steps fail because of #12.

@samuelcolvin
Copy link
Owner

this looks great. Please rebase to fix the errors and update the readme.

@homeworkprod
Copy link
Contributor Author

Done. Is this what you had in mind regarding the README?

@samuelcolvin samuelcolvin merged commit 33a4b26 into samuelcolvin:master Oct 2, 2020
@samuelcolvin
Copy link
Owner

great. I guess I'l release a new version. Is there anything you need?

@homeworkprod homeworkprod deleted the pretty branch October 2, 2020 16:38
@homeworkprod
Copy link
Contributor Author

Excellent!

I don't think I need anything else.

@samuelcolvin
Copy link
Owner

deploying now.

@homeworkprod
Copy link
Contributor Author

Thanks a lot. Works as expected!

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.

None yet

2 participants