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

Adds minimal support for Array of Tables (AoT) #15

Merged
merged 12 commits into from
Oct 19, 2021

Conversation

abravalheri
Copy link
Contributor

Inline tables are a cool feature of the TOML, and very useful as elements of an array.

However there are some circumstances that they are explicitly discouraged by spec itself.

For example when dumping a list of dicts that in turn have lists inside of them, tomli-w will introduce line breaks inside of inline tables, which is not recommended by the spec

This situation was first described in issue #12.

Please notice this usage pattern is not that uncommon. We can find examples of this nesting in the wild, like the ones used in mypy configuration:

https://mypy.readthedocs.io/en/stable/config_file.html#using-a-pyproject-toml-file

Inline tables are a cool feature of the TOML, and very useful as
elements of an array.

However there are some circumstances that they are explicitly
discouraged by spec itself.

For example when dumping a list of dicts that in turn
have lists inside of them, `tomli-w` will introduce line breaks
inside of inline tables, which is not recommended by the spec

This situation was first described in issue hukkin#12.

Please notice this usage pattern is not that uncommon. We can find
examples of this nesting in the wild, like the ones used in `mypy`
configuration:

https://mypy.readthedocs.io/en/stable/config_file.html#using-a-pyproject-toml-file
@abravalheri abravalheri marked this pull request as ready for review October 15, 2021 17:12
@codecov-commenter
Copy link

codecov-commenter commented Oct 15, 2021

Codecov Report

Merging #15 (34b8bc8) into master (bb7dc86) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #15   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           92       105   +13     
  Branches        26        30    +4     
=========================================
+ Hits            92       105   +13     
Impacted Files Coverage Δ
tomli_w/_writer.py 100.00% <100.00%> (ø)

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 bb7dc86...34b8bc8. Read the comment docs.

tomli_w/_writer.py Outdated Show resolved Hide resolved
Copy link
Owner

@hukkin hukkin left a comment

Choose a reason for hiding this comment

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

Thanks a bunch! Generally looking very good. I added a couple notes

tomli_w/_writer.py Outdated Show resolved Hide resolved
tomli_w/_writer.py Outdated Show resolved Hide resolved
tomli_w/_writer.py Outdated Show resolved Hide resolved
tomli_w/_writer.py Outdated Show resolved Hide resolved
tomli_w/_writer.py Outdated Show resolved Hide resolved
Co-authored-by: Taneli Hukkinen <3275109+hukkin@users.noreply.github.com>
tomli_w/_writer.py Outdated Show resolved Hide resolved
hukkin and others added 5 commits October 19, 2021 13:21
There might be a chance any-not is slightly more costly than not-all
(although if it exists, the performance difference should be negligible)
@abravalheri
Copy link
Contributor Author

abravalheri commented Oct 19, 2021

Thank you very much for the review @hukkin.

I added the missing test and also fixed somethings regarding simply committing the suggestions via GitHub interface 😝

(I also reverted back to not-all instead of all-any, just because there might be a negligible performance penalty of flipping each Boolean but I admit that is very much a micro-optimisation that lacks benchmarking).

tomli_w/_writer.py Outdated Show resolved Hide resolved
tomli_w/_writer.py Outdated Show resolved Hide resolved
# In the following line we use `repr(obj)` as an approximation for the
# TOML representation of an inline-table when `obj` is a dict,
# (for the purposes of roughly estimating the line length)
return len(repr(obj)) < LONG_LINE_HEURISTIC
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
return len(repr(obj)) < LONG_LINE_HEURISTIC
return len(format_literal(obj)) < LONG_LINE_HEURISTIC

Would this work? It's probably more expensive, but would probably allow us to be actually precise about the line length

Copy link
Contributor Author

@abravalheri abravalheri Oct 19, 2021

Choose a reason for hiding this comment

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

I suppose it would (I haven't checked for infinite recursion though)
It is just important to have in mind that format_literal will still not consider the indentation inside an array (so in the end the condition will be implicitly LONG_LINE_HEURISTIC + indentation_len)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added your suggestion, the tests work so I suspect the recursion is fine 😝

Please feel free to revert the last commit if you change your mind about the precision/cost tradeoff 😄.

# TOML representation of an inline-table when `obj` is a dict,
# (for the purposes of roughly estimating the line length)
return len(repr(obj)) < LONG_LINE_HEURISTIC
max_len = LONG_LINE_HEURISTIC - (INDENT_LENGTH * nest_level)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I think a nest_level argument (despite not necessary), makes more obvious and easier to understand.

tomli_w/_writer.py Outdated Show resolved Hide resolved
@hukkin hukkin merged commit 4990642 into hukkin:master Oct 19, 2021
@hukkin
Copy link
Owner

hukkin commented Oct 19, 2021

Thanks a lot for this! Tell me if you need a release soon and I'll make one. Otherwise I may wait until Python 3.6 removal or another feature.

@abravalheri
Copy link
Contributor Author

Thanks @hukkin.

I am waiting for this feature for my https://github.com/abravalheri/ini2toml project, but on the other hand I also have to wait for a couple of fixes in atoml (a more maintained fork of tomlkit). So I am in no rush right now...

@hukkin
Copy link
Owner

hukkin commented Oct 19, 2021

👍 the CI of this PR should push v0.4.0 to PyPI for you #19

Thanks once more!

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