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

Support Decimal and MutableMapping codecs, add a multiline-Array indent control #294

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

goodboy
Copy link

@goodboy goodboy commented May 25, 2023

In an attempt to resolve the feature request issues i submitted:


Summary:

  • add a Array._multiline_indent: str and expose it through a indent: str kwarg to .multiline().
    • main purpose is to be able to style the indent level for user preferred indentation of say a multiline array of tables B)
  • add a branch to the .items.item() encoder to handle decimal.Decimal and match against MutableMapping instead of dict.
  • tests for all of it!

Possibly still todo?

  • Do i need to submit more for Decimal and MutableMapping changes?
    • currently i added 2 new tests for decimal encoding in the best places i could find.
    • don't know if mut map tweaks require anything extra other then maybe some tests maybe using a custom type? ( i feel like that is just testing the protocol and thus stdlib stuff 😂 )
  • tests for the inline Array indent parameter:

@goodboy
Copy link
Author

goodboy commented May 25, 2023

Bah, going through rebase to master 🏄🏼

For `decimal.Decimal` we can just encode as a `String` and leave it up
to the user how to decode the value on load. For `dict` it's quite handy
to just render any type that quacks like a `MutableMapping` to a table.

Resolves python-poetry#288 and python-poetry#289 but still needs tests!
Simply casts to `Decimal` then calls `item()` on it as normal for most
basic types.
Ensure it both unwraps as a `String` (or `str`) and that the
`api.decimal()` encoder routine does the same.
@goodboy goodboy force-pushed the decdecode_tableindent_mutmaps branch from 9823394 to 8e0239a Compare May 25, 2023 17:42
@goodboy
Copy link
Author

goodboy commented May 25, 2023

There hopefully i got all the latest changes in now B)

@goodboy goodboy changed the title Support Decimal and MutableMapping decode, add a multiline-Array indent control Support Decimal and MutableMapping codecs, add a multiline-Array indent control May 25, 2023
@goodboy
Copy link
Author

goodboy commented May 25, 2023

Also, if this patch is likely to never be accepted or needs to be changed to be accepted please let me know asap since we'd much rather not have to maintain our own fork :)

@frostming
Copy link
Contributor

frostming commented May 27, 2023

a better way to support this and similar cases is to allow passing a custom encoder to item() function.

i don't think we need round trip

@goodboy
Copy link
Author

goodboy commented Jun 19, 2023

@frostming ah ok, I'm fine to change it to do that.

(also sorry i missed your comment don't have my email hooked up rn for GH 😂)

is to allow passing a custom encoder to item() function.

can you show an example that might already exist for doing this btw in the code base and for which feature addition out of the issues I made does this apply? All?


i don't think we need round trip

Also can you clarify wym by that?

@frostming
Copy link
Contributor

frostming commented Jun 20, 2023

You can convert a decimal to a float representation in TOML, but can't convert it back, and Decimal isn't listed as the data types defined by TOML spec, so it's not worth to expose a new api and item type for it.

Instead, I'd propose something like this:

import tomlkit
a = Decimal('34.56')

def my_encoder(o):
    if isinstance(o, Decimal):
        return tomlkit.float_(o)
    raise ValueError(f"Invalid type {type(value)}")

item = tomlkit.item(a, encoder=my_encoder)   # a Float object
v = {"a": Decimal("34.56")}
print(tomlkit.dumps(v, encoder=my_encoder)
# a = 34.56

In this way users can convert custom types, not only Decimal, to TOML items in tomlkit. And it is clearer that users can't convert it back(parse anything as Decimal)

def multiline(
self,
multiline: bool,
indent: str = " "*4,
Copy link
Contributor

@frostming frostming Jun 20, 2023

Choose a reason for hiding this comment

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

Prefer to use int indent=4, to prevent passing arbitrary strings.

@goodboy
Copy link
Author

goodboy commented Jun 27, 2023

so it's not worth to expose a new api and item type for it.
In this way users can convert custom types, not only Decimal, to TOML items in tomlkit. And it is clearer that users can't convert it back(parse anything as Decimal)

@frostming works for me. So I guess i'll just drop all the Decimal related changes then and rebase this branch a bit.

Any other quiffs and/or do you have any suggestions for the tests I still need to do?

@frostming
Copy link
Contributor

@goodboy yes, I've proposed a change in #296

@goodboy
Copy link
Author

goodboy commented Jul 10, 2023

@frostming thanks for the follow up!

i will hopefully get back to this PR this week. Will check out that landed PR and change our stuff to match first, then come back here and drop the unneeded history.

Still kinda looking for a little guidance on how thorough the new tests for the multi-line arrays should be?
i have some basic ideas but don't want to push too deep if y'all aren't that concerned about it.

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.

2 participants