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

Add argument none_value for None representation in loading and dumping #53

Merged
merged 16 commits into from
Jun 24, 2024

Conversation

pwwang
Copy link
Contributor

@pwwang pwwang commented Nov 12, 2022

Related: #23, toml-lang/toml#30

  • load/loads
    # By default, nothing should be loaded as None, since it doesn't exist in TOML
    rtoml.loads('x = "null"') == {'x': "null"}
    rtoml.loads('x = ""') == {'x': ""}
    # Now with the PR, we can tell what to be interpreted as None
    rtoml.loads('x = "null"', none_value="null") == {'x': None}
    rtoml.loads('x = ""', none_value="") == {'x': None}
  • dump/dumps
    # By default, None dumps into "null"
    rtoml.dumps({"x": None}) == 'x = "null"\n'
    # Now with the PR, we can control what None should be dumped into
    rtoml.dumps({"x": None}, none_value="@None") == 'x = "@None"\n'

Reproducibility between loading and dumping using the same none_value:

s = 'x = "py:None"\n'
rtoml.loads(s, none_value="py:None") == {'x': None}
rtoml.dumps({'x': None}, none_value="py:None") == s

s = {'x': None}
rtoml.dumps(s, none_value="py:None") == 'x = "py:None"\n'
rtoml.loads('x = "py:None"\n', none_value="py:None") == s

The PR modifies the visitor at rust end, and no re-visit is needed to covert the values at python end (like suggested by #23). So it barely affects the speed.

I think the only "drawback" is that only string None-representation is supported. You can't convert 0 (integer) to None, for example, while loading, or vice versa while dumping.

You can also omit None values or items that are associated with None value with omit_none=True. See #23 (comment)

rtoml.dumps({'test': None}, omit_none=True) == ''
rtoml.dumps({'test': [1, None, 2]}, omit_none=True) == 'test = [1, 2]\n'
rtoml.dumps({'test': (1, None, 2)}, omit_none=True) == 'test = [1, 2]\n'
rtoml.dumps({'test': {'x': [None, {'y': [1, None, 2]}], 'z': None}}, omit_none=True)  # gives
"""
[[test.x]]
y = [1, 2]
"""
# But with omit_none=False
rtoml.dumps({'test': {'x': [None, {'y': [1, None, 2]}], 'z': None}}, omit_none=False, pretty=True)   # gives
"""
[test]
z = 'null'

[[test.x]]
y = [
    1,
    'null',
    2,
]

@pwwang pwwang changed the title Add argument None representation in loading and dumping Add argument none for None representation in loading and dumping Nov 12, 2022
@codecov
Copy link

codecov bot commented Nov 12, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (1d0b60e) to head (5fc12d6).

Current head 5fc12d6 differs from pull request most recent head d6302af

Please upload reports for the commit d6302af to get more accurate results.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #53   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           29        29           
  Branches         5         5           
=========================================
  Hits            29        29           

@samuelcolvin
Copy link
Owner

Looking good, suggestions:

  • Rename the argument to none_value
  • make none_value optional, default None when deserialising

@pwwang pwwang changed the title Add argument none for None representation in loading and dumping Add argument none_value for None representation in loading and dumping Nov 12, 2022
@pwwang
Copy link
Contributor Author

pwwang commented Nov 12, 2022

Revised.

@samuelcolvin
Copy link
Owner

This looks awesome, I'll review properly when I'm at a computer.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise LGTM.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/ser.rs Outdated Show resolved Hide resolved
src/ser.rs Outdated Show resolved Hide resolved
src/ser.rs Outdated Show resolved Hide resolved
src/ser.rs Outdated Show resolved Hide resolved
src/ser.rs Outdated Show resolved Hide resolved
src/ser.rs Outdated Show resolved Hide resolved
tests/test_benchmarks.py Outdated Show resolved Hide resolved
@pwwang
Copy link
Contributor Author

pwwang commented Nov 12, 2022

The omit_none argument was added at 3be8586.

I know it's a sort of different functionality but it costs tiny modifications on the current code base. I can submit a separate PR if you want.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

a few more things to change.

We also need to update the readme.

rtoml/__init__.py Outdated Show resolved Hide resolved
src/ser.rs Outdated Show resolved Hide resolved
src/ser.rs Outdated Show resolved Hide resolved
src/ser.rs Outdated Show resolved Hide resolved
src/ser.rs Outdated Show resolved Hide resolved
src/ser.rs Outdated Show resolved Hide resolved
src/ser.rs Outdated Show resolved Hide resolved
@pwwang
Copy link
Contributor Author

pwwang commented Nov 13, 2022

As for the readme file, I think this None-value handling could be a selling point. Another point is that, in my opinion, you can state that rtoml is THE fastest TOML library for python, based on the benchmarking from https://github.com/pwwang/toml-bench

@samuelcolvin
Copy link
Owner

As for the readme file, I think this None-value handling could be a selling point. Another point is that, in my opinion, you can state that rtoml is THE fastest TOML library for python, based on the benchmarking from https://github.com/pwwang/toml-bench

I agree on both, we should say "at the time of writing" just in case. Feel free to update here or in another PR.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

Looking good, I noticed you still need to update table_key to use none_value, also could you add tests for {None: 'foobar'}, both with none_value='custom-value' and none_value=None?

That and readme updates, and I think we're done. 🎉

@caniko
Copy link

caniko commented May 6, 2023

What happened to this PR?

@samuelcolvin
Copy link
Owner

What happened to this PR?

I got very busy 😄.

Fixing now.

@samuelcolvin samuelcolvin enabled auto-merge (squash) June 24, 2024 10:55
@samuelcolvin samuelcolvin merged commit 092eff2 into samuelcolvin:main Jun 24, 2024
10 checks passed
@samuelcolvin
Copy link
Owner

Thanks so much for this @pwwang.

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