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

TOML support #69

Merged
merged 13 commits into from
Sep 20, 2024
Merged

TOML support #69

merged 13 commits into from
Sep 20, 2024

Conversation

vanodevium
Copy link
Contributor

@Crell

I want to ask for your help because I'm having a bit of a hard time figuring out the tests for this format.

There are two main problems:

  • TOML allows you to format float values ​​as text values ​​to preserve the desired formatting (for example, trailing zero are discarded in the PHP, and the TOML requires it)
  • TOML does not like null values ​​at all, there is a rather unpleasant story with it.

Could you look at it from your side and tell me how to write the tests correctly?

Thank you!

@Crell
Copy link
Owner

Crell commented Aug 12, 2024

Thanks! It looks like you're doing the right thing overall, especially for the test class. (Though there's a PHPStan issue to address there.)

For the float/string point, Serde supports both strict and weak modes, but you have to opt-in to weak mode, and what weak/strict mode means is up to the formatter. It looks like TOML requires being in weak mode for at least floats. Probably the easiest thing to do there is copy the float read/write methods into the TomlFormatter and adjust them to always operate as though the field was in weak mode. (I was doing that in my earlier attempts at XML support.)

As for nulls, can you clarify what "there is a rather unpleasant story with it" means? Many formats support null, so Serde has to, even though I detest null and it's the source of most of the bug reports I've gotten. 😄 What does TOML want you to do in case of a null value? Simply omit it?

@vanodevium
Copy link
Contributor Author

TOML didn't have any variants of null. In general.

So many encoders did next things:

  • if there is null in array - exception
  • if null is value for some key in the key-value structure - omitting for this key

@Crell
Copy link
Owner

Crell commented Aug 12, 2024

OK, then here, we'd probably want to also override serializeNull() and turn it into a no-op, so it just gets omitted. That would handle the write side, at least. For reading... it gets tricky, because the user-supplied defaults get added to the mix, too, so there's a number of different ways it could go. Let's do the write side first and see how badly it breaks and determine what to do next. 😄

@vanodevium vanodevium changed the title wip: TOML support TOML support Aug 13, 2024
@vanodevium
Copy link
Contributor Author

@Crell Hi!

I did my best. Please check from you side.

@vanodevium vanodevium requested a review from Crell August 13, 2024 15:54
@Crell
Copy link
Owner

Crell commented Aug 13, 2024

Well that version issue is going to be fun. That will probably require doing some interesting things in the GHA, combined with some PHPUnit version checks. Leave the first to me, but if you can set the Toml tests to not run on PHP <8.2, that would be helpful.

Did you try overriding serializeNull() to a no-op? That may mean we don't need to skip the tests. I'll try to have a closer look later today/tomorrow.

@vanodevium
Copy link
Contributor Author

@Crell

I have added version compare for >=8.2

About serializeNull(), serializeFloat(): yes I did, but no effect.

Maybe you will manage to deal with it better.

@vanodevium
Copy link
Contributor Author

Oh I see. Version check didn't help.

@vanodevium
Copy link
Contributor Author

If only we can add --ignore-platform-req=php into testing environment...

@vanodevium
Copy link
Contributor Author

@Crell

Special promo: I downgraded PHP version only for compatibility.
Without braking changes, of course.

@vanodevium
Copy link
Contributor Author

@Crell just ping

@Crell
Copy link
Owner

Crell commented Aug 25, 2024

I'm back. I've looked into this a bit, and there's two issues.

One, turns out there's a bug in ArrayBasedDeformatter::deserializeSequence(). It doesn't check for strict/weak mode on the values in an array, so it will always reject ['1', '2'] going into an array<int>, even in weak mode. I think I have a fix for that but will need to test it separately.

Two, the TOML library is returning float as strings, always. That means any float values, at least in arrays (all I've found so far), will be a type mismatch. It's possible to address this in the deformatter, but is there a reason it cannot be handled in the TOML library itself, the way it is in YAML, to convert a float-esque string into a float?

@vanodevium
Copy link
Contributor Author

@Crell

The TOML specification says that float values can parsed as string. This is done so that languages ​​that automatically round floats can return a string that shows sufficient precision. Unfortunately, PHP is just such a language.

Example:

longpi = 3.141592653589793
neglongpi = -3.141592653589793

PHP returns 3.1415926535898 and -3.1415926535898 accordingly.

pay attention, it rounds ...9793 into ...98

@vanodevium
Copy link
Contributor Author

@Crell

Anyway, I've updated the library and now it's possible to force return values ​​as floats.

So, now it's you turn with "issue one".

Thanx!

@Crell
Copy link
Owner

Crell commented Sep 6, 2024

Fixed the weak mode issue in #70. Still working on some other test fails here.

@Crell
Copy link
Owner

Crell commented Sep 6, 2024

I think that does it. I'll leave this here until @vanodevium gives it a thumbs up, but then I think we can merge and tag a 1.3 release, since this is kind of a big deal. 😄

@Crell Crell merged commit 4e1764a into Crell:master Sep 20, 2024
6 checks passed
@Crell
Copy link
Owner

Crell commented Sep 20, 2024

Tagged and released. Thanks, @vanodevium!

@vanodevium vanodevium deleted the toml branch September 20, 2024 08:50
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