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

Dont dump none entity #31

Closed
wants to merge 18 commits into from
Closed

Conversation

zhiburt
Copy link

@zhiburt zhiburt commented Nov 6, 2021

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

closes #23

@codecov
Copy link

codecov bot commented Nov 6, 2021

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
Copy link
Owner

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
Copy link
Author

zhiburt commented Nov 6, 2021

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
Copy link
Author

zhiburt commented Nov 6, 2021

@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
Copy link
Owner

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

@zhiburt
Copy link
Author

zhiburt commented Dec 5, 2021

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.

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 looks good.

rtoml/__init__.py Outdated Show resolved Hide resolved
rtoml/_rtoml.pyi Outdated Show resolved Hide resolved
src/lib.rs Outdated
} else if let Ok(key) = $key.str() {
let key = key.to_string();
$map.serialize_key(&key)?;
if self.omit_none && $value.is_none() {
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

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, I think makes sense.

src/lib.rs Outdated Show resolved Hide resolved
"Dictionary key is not a string: {:?}",
$key
)));
let ignore_none_entry = !self.include_none && ($value.is_none() || $key.is_none());
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Because it would break the tests.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
rtoml/__init__.py Outdated Show resolved Hide resolved
src/lib.rs Outdated
}

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> {
Copy link
Owner

Choose a reason for hiding this comment

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

not required.

Copy link
Author

@zhiburt zhiburt May 13, 2022

Choose a reason for hiding this comment

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

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
Copy link
Contributor

pwwang commented May 12, 2022

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.

zhiburt and others added 5 commits May 13, 2022 14:57
Co-authored-by: Samuel Colvin <[email protected]>
Co-authored-by: Samuel Colvin <[email protected]>
Signed-off-by: Maxim Zhiburt <[email protected]>
Co-authored-by: Samuel Colvin <[email protected]>
Signed-off-by: Maxim Zhiburt <[email protected]>
Co-authored-by: Samuel Colvin <[email protected]>
Signed-off-by: Maxim Zhiburt <[email protected]>
@samuelcolvin
Copy link
Owner

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

@samuelcolvin
Copy link
Owner

as agreed above, we would need a different implementation.

Also the logic here has changed significantly since #51.

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.

Conversion of None to "null" string inconsistency
3 participants