Dont dump none entity #31

zhiburt · 2021-11-06T11:24:56Z

Hi there,
not an expert in python and neither in your library,
but let's say coincidentally ran into it.

closes #23

codecov · 2021-11-06T11:36:32Z

Codecov Report

Merging #31 (0dfed85) into main (a9ac770) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #31   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           28        28           
  Branches         5         5           
=========================================
  Hits            28        28

samuelcolvin · 2021-11-06T11:37:21Z

See my comment #23 (comment) I don't agree with this approach.

If were to go with this approach we would need:

to omit None values from lists too
to hide it behind a kwarg which defaults to off as described in my comment above

zhiburt · 2021-11-06T11:44:39Z

If were to go with this approach we would need to omit None values from lists too

Do you mean it shall be done in order to accept these changes or the approach is invalid at its core so it'd better to be left as it is?

zhiburt · 2021-11-06T12:05:02Z

@samuelcolvin I've covered sequences.

But I need to notice that
tuple and list behavior is different, so
please take a look at d7eb3ee because I do not certain in this regard

samuelcolvin · 2021-12-05T10:37:15Z

This needs to be hidden behind a kwarg as I mentioned above.

zhiburt · 2021-12-05T14:53:35Z

Hi

This needs to be hidden behind a kwarg as I mentioned above.

I am not an expert in python as I've already mentioned but I introduced a optional argument.

samuelcolvin

otherwise looks good.

rtoml/__init__.py

rtoml/_rtoml.pyi

samuelcolvin · 2021-12-07T23:02:16Z

src/lib.rs

-                } else if let Ok(key) = $key.str() {
-                    let key = key.to_string();
-                    $map.serialize_key(&key)?;
+                if self.omit_none && $value.is_none() {


for clarity, and to avoid too many changes, I would prefer this check was done inside the $key.is_none() if case, rather than as a separate block.

I've tried to eliminate it but as I see there's no easy way?

I changed it a bit e7b8ddd
But it still... not as plain as it'd better be :)

samuelcolvin · 2021-12-07T23:05:11Z

tests/test_dump.py

+@pytest.mark.parametrize(
+    'input_obj,output_toml',
+    [
+        ({'key': None}, ''),


what about None as the key: {None: 1}?

Good catch.
I did fix it now it doesn't serialize anything.

samuelcolvin

otherwise, I think makes sense.

src/lib.rs

samuelcolvin · 2022-05-09T15:04:15Z

src/lib.rs

-                        "Dictionary key is not a string: {:?}",
-                        $key
-                    )));
+                let ignore_none_entry = !self.include_none && ($value.is_none() || $key.is_none());


I suspect we can do this in a more effecient way, e.g. only check instead the $key.is_none() logic etc.

Do you mean only check $key.is_none()?

Because it would break the tests.

README.md

rtoml/__init__.py

samuelcolvin · 2022-05-09T15:07:45Z

src/lib.rs

 }

-impl<'p, 'a> Serialize for SerializePyObject<'p, 'a> {
+impl<'p> SerializePyObject<'p> {
+    fn new(py: Python<'p>, obj: &'p PyObject, include_none: &PyObject) -> PyResult<Self> {


not required.

Maybe not; but isn't it better with it?:)

I mean personally I like 2calls to SerializePyObject::new

rather then 2 constructors

SerializePyObject { py, include_none, obj: obj.extract(py)?, }

pwwang · 2022-05-12T17:54:29Z

I like the second bullet point from the original proposal #23 (comment). That is, to allow something like a none_string or none_repr, which defaults to "null", to dump None into a specified string representation.

I am Okay with leaving it to the developers to handle loading the none_repr, or adding the same argument for .load() or .loads() to, for example, turn "null" back to None. The latter option is definitely handy as we don't need to loop over all values to convert the none_repr string to None later.

A use case would look like this:

import rtoml

config = {"addr": "localhost", "port": None}
toml_str = rtoml.dumps(config, none_repr="@none None")  # in case "null" is not special enough
# addr = "localhost"
# port = "@none None"

myconfig = rtoml.loads(toml_str, none_repr = "@none None")
# {"addr": "localhost", "port": None}

This use case can be found here: https://dynaconf.readthedocs.io/en/docs_223/guides/environment_variables.html#precedence-and-type-casting

dynaconf is a very popular configuration management library, with 2.4K stars, that is currently using toml. With this implementation, we have a very good reason to ask the authors to switch it to rtoml.

We can also allow none_repr to be None, which then falls back to this implementation with include_none = False.

Since we decide to interpret None, it doesn't hurt to allow a custom representation of it when being dumped and turn it back when the representation is loaded. This will save some extra loops for the developers to handle None value.

With this, we can regard it as a superset of TOML, as we are not breaking the standard TOML specification, but just extending it to be able to be more powerful.

I know that a lot of people are wanting to see this with TOML:

And I believe this will raise rtoml's popularity.

Co-authored-by: Samuel Colvin <samcolvin@gmail.com>

Signed-off-by: Maxim Zhiburt <zhiburt@gmail.com> Co-authored-by: Samuel Colvin <samcolvin@gmail.com>

Signed-off-by: Maxim Zhiburt <zhiburt@gmail.com>

samuelcolvin · 2022-05-13T13:03:17Z

I agree with @pwwang but logic for detecting a "none value" when parsing toml could wait for a separate PR.

samuelcolvin · 2022-11-10T16:37:14Z

as agreed above, we would need a different implementation.

Also the logic here has changed significantly since #51.

zhiburt added 4 commits November 6, 2021 14:21

Add .env folder to gitignore

39a1d56

Dont serialize map entity whereas valus is None

9ff9816

Add tests for None value in maps

fc35a3c

Add more tests for None value in the map

da4497c

Fix lint warnings

0462456

zhiburt added 2 commits November 6, 2021 15:02

Ommit None values in sequences

67ab23d

Add test for None in sequences

d7eb3ee

zhiburt added 3 commits December 5, 2021 17:45

Add omitNone argument

1d55b16

Use omitNone flag

f81d051

Rename omitNone to omit_none

772abe4

samuelcolvin reviewed Dec 7, 2021

View reviewed changes

zhiburt added 2 commits December 8, 2021 20:44

Address PR issues

1732cd6

Update README.md

2aebd0c

zhiburt requested a review from samuelcolvin December 8, 2021 17:51

Refactoring

e7b8ddd

samuelcolvin reviewed May 9, 2022

View reviewed changes

pwwang mentioned this pull request May 12, 2022

Empty table key should be allowed #39

Closed

zhiburt and others added 5 commits May 13, 2022 14:57

Update README.md

bf7d47c

Co-authored-by: Samuel Colvin <samcolvin@gmail.com>

Update rtoml/__init__.py

8402fad

Co-authored-by: Samuel Colvin <samcolvin@gmail.com>

Refactoring lib.rs

c1b8123

Signed-off-by: Maxim Zhiburt <zhiburt@gmail.com> Co-authored-by: Samuel Colvin <samcolvin@gmail.com>

Update README.md

f37a0aa

Signed-off-by: Maxim Zhiburt <zhiburt@gmail.com> Co-authored-by: Samuel Colvin <samcolvin@gmail.com>

Fix lint warnings

0dfed85

Signed-off-by: Maxim Zhiburt <zhiburt@gmail.com>

samuelcolvin closed this Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dont dump none entity #31

Dont dump none entity #31

zhiburt commented Nov 6, 2021 •

edited

Loading

codecov bot commented Nov 6, 2021 •

edited

Loading

samuelcolvin commented Nov 6, 2021

zhiburt commented Nov 6, 2021 •

edited

Loading

zhiburt commented Nov 6, 2021

samuelcolvin commented Dec 5, 2021

zhiburt commented Dec 5, 2021

samuelcolvin left a comment

samuelcolvin Dec 7, 2021

zhiburt Dec 8, 2021

zhiburt Dec 8, 2021

samuelcolvin Dec 7, 2021

zhiburt Dec 8, 2021

samuelcolvin left a comment

samuelcolvin May 9, 2022

zhiburt May 13, 2022

samuelcolvin May 9, 2022

zhiburt May 13, 2022 •

edited

Loading

pwwang commented May 12, 2022

samuelcolvin commented May 13, 2022

samuelcolvin commented Nov 10, 2022

Dont dump none entity #31

Dont dump none entity #31

Conversation

zhiburt commented Nov 6, 2021 • edited Loading

codecov bot commented Nov 6, 2021 • edited Loading

Codecov Report

samuelcolvin commented Nov 6, 2021

zhiburt commented Nov 6, 2021 • edited Loading

zhiburt commented Nov 6, 2021

samuelcolvin commented Dec 5, 2021

zhiburt commented Dec 5, 2021

samuelcolvin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelcolvin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhiburt May 13, 2022 • edited Loading

Choose a reason for hiding this comment

pwwang commented May 12, 2022

samuelcolvin commented May 13, 2022

samuelcolvin commented Nov 10, 2022

zhiburt commented Nov 6, 2021 •

edited

Loading

codecov bot commented Nov 6, 2021 •

edited

Loading

zhiburt commented Nov 6, 2021 •

edited

Loading

zhiburt May 13, 2022 •

edited

Loading