rtoml icon indicating copy to clipboard operation
rtoml copied to clipboard

Dont dump none entity

Open zhiburt opened this issue 4 years ago • 8 comments

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

closes #23

zhiburt avatar Nov 06 '21 11:11 zhiburt

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           

codecov[bot] avatar Nov 06 '21 11:11 codecov[bot]

See my comment https://github.com/samuelcolvin/rtoml/issues/23#issuecomment-917640979 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

samuelcolvin avatar Nov 06 '21 11:11 samuelcolvin

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 avatar Nov 06 '21 11:11 zhiburt

@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

zhiburt avatar Nov 06 '21 12:11 zhiburt

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

samuelcolvin avatar Dec 05 '21 10:12 samuelcolvin

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.

zhiburt avatar Dec 05 '21 14:12 zhiburt

I like the second bullet point from the original proposal https://github.com/samuelcolvin/rtoml/issues/23#issuecomment-917640979. 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:

  • https://twitter.com/mitsuhiko/status/1346455555435593729
  • https://github.com/toml-lang/toml/issues/30
  • https://github.com/uiri/toml/issues/346
  • and so on ...

And I believe this will raise rtoml's popularity.

pwwang avatar May 12 '22 17:05 pwwang

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

samuelcolvin avatar May 13 '22 13:05 samuelcolvin

as agreed above, we would need a different implementation.

Also the logic here has changed significantly since #51.

samuelcolvin avatar Nov 10 '22 16:11 samuelcolvin