Unify Psych's API

[sorryeh]

Goal

Unify Psych's API

Why

The API is not clear, neither consistent. Certain methods take optional positional arguments, which conflict with other keyword arguments on certain inputs. Some other methods don't have access to features implemented, just not available from the API (ex: fallback)

While going through the code, you can tell certain things were added for specific purpose and where not implemented consistently with the rest of the API, thus we have a mixed API.

These changes make the API consistent which makes it simpler to navigate and memorize.
However, it does break the current API, which would indicate a MAJOR release if this project follows SemVer?

NOTE: The current signature of multiple methods requires positional arguments, even thought these arguments are optional; yet other optional arguments are passed as keyword arguments.
It is unfortunate, users of the API need to pass all optional positional arguments to access later ones.
Making them keyword arguments fixes this usability issue, as well as some previous issues with passing certain inputs ({} in a positional argument vs. keyword arguments)

How

Kept all the method's first necessary argument as a positional arg. Everything else as been made keyword arguments.

All available features to a method as been exposed via keyword arguments and using sane defaults.

Removed the concept of FALLBACK internal Struct and made the fallback mechanism more sensical by checking a parse result and returning the fallback then, instead of returning from parse and then having to handle the .to_ruby call on result (which was unique to load.
Now all fallbacks are handled the same way by all methods.

Removed fallback from parse and instead returns the default false which would trigger returning a fallback at the method level for individual methods using parse.
(If I'm missing a use case for strictly using parse with a fallback, I can add it back there too, default to false but keep the new consistent fallback behaviour.

Modified all the tests that used positional arguments to use keyword arguments.

Added a doc line for the load methods that didn't mention failing on nil. This is implicit in the code and explicit in the unit tests, but never documented for end-users.

First time contributing here, so if I'm doing something wrong or if I lack a certain context, I'd love to know how I can be helpful/improve/learn.

Cheers! 🙂 🚢

[sorryeh]

bump @stomar @tenderlove

I don't know if I'm supposed to expect a reply soonish? I see the latest activity was ~1 week or late December

Cheers :)

[yuki24]

I wonder if it makes sense to add a new test that just uses the deprecated API to make sure it's deprecated but still useable.

cc @tenderlove

[tenderlove]

This looks good, but I think we should do a few things. First, I don't think the API has changed in a long time so I'm afraid to break it too much (lots of it was inherited from Syck). If this API is fully backwards compatible, we should release a version that allows the change but doesn't warn. Then release a version that does warn. Then release a version that removes the backward compatible code. I like these changes, but it will take a while to ship.

We should also add some tests to make sure the existing API still works (as @yuki24 said)

[sorryeh]

Thanks @tenderlove, I'll:

• test the existing API
• remove the warnings
• create another branch with the deprecation warnings as a follow up for a future release

[sorryeh]

@tenderlove @yuki24 All changes made. Cheers!

[sorryeh]

Bump

[yuki24]

Thanks! This looks good to me 👍

[sorryeh]

Thanks for the quick review Yuki!

When @tenderlove gets the time to give his input, we can ship this 🎉

[stomar]

Will there be a gem release of the last version before this change? (Just in case something has been overlooked...)

[stomar]

It‘s not clear to me under what conditions the fallback will be returned here.

Psych.load_stream "", fallback: 42  # => []
# neither the specified value nor the default (false) is used in this case

# whereas
Psych.load "", fallback: 42  # => 42

[sorryeh]

👀 Looking into this, was away on Holiday sorry for late response

[sorryeh]

Just came back to it.

Figured out the discrepancy with the fallback when calling load_stream (return: [])

Basically, parse_stream (called in load_stream) returns an empty array for an empty YAML value. Thus, in this case, the fallback isn't called because it uses the empty array from parse_stream as the result.

I'll investigate to see whether this is a normal case, a unique case and/or a bug. I'll see how to proceed and update as necessary.

The empty array represents an empty array of children for the parse stream. Thus, the empty array for load_stream indicates no return --> fallback.

For now, the appropriate fix is to return fallback on empty array. It requires no major changes and is consistent with previous behaviour

[sorryeh]

Bump

ready to 🚢 ❤️ let me know if you are 👍
@stomar @tenderlove @yuki24

[stomar]

1. Still considering load_stream: when I'm not mistaken, the PR would change the return value for Psych.load_stream("") from [] to false (the default fallback), which might break existing code that iterates over the return value. This should be considered.

2. This also makes me wonder whether the current behavior (aka "existing API") is sufficiently covered by tests. I assume there is no test case for Psych.load_stream(""), and there might be more edge cases that are not covered. (I had/have no time to thoroughly review all changes.)

[sorryeh]

@stomar Oh, for 1. I completely forgot about that.. Let me change it

2. Yes the current test coverage isn't great for these corner cases. I noticed there isn't any test coverage for a lot of these. I added some, I can check to add some tests that expect the existing API's behaviour and ensure they pass as expected (like "")

These default are all different for the current behaviour/implementation.
Do we want to keep this super fragmented behaviour for the special case "" or change them to be consistent? (I assume changing them would require a MAJOR bump..? It does seem like unplanned differences thought, especially since it isn't tested for. What do you think? @tenderlove @stomar)

[stomar]

I guess changing any behavior would be a separate step.

An empty array seems like a reasonable fallback, though, iteration behaves properly; and always returning an array and not different kinds of objects is also consistent in a way. Not sure what consistency would weigh more. BTW, another thought: why false and not nil...?
(Just some thoughts of a tiny contributor without any insight into the bigger picture.)

[sorryeh]

I added, which hsbt merged, some new tests (#366) covering the current behaviour of default fallbacks on master branch

I'll look into either modifying the current behaviour to return the same sane value for all load/parse methods

OR

keep the current disjointed behaviour AND push a new default fallback in a follow up PR. (_Add deprecation warnings for it in --> #360 and follow up MAJOR bump PR?)

What do you think @tenderlove @yuki24 @hsbt ? (Any other alternative to go forward in regards with default fallbacks and this PR in general? 😄

[sorryeh]

@stomar the reason for false is what is used as a default for most recent fallback "features" in the repo

I could change it to anything considering it's a breaking change anyway, I'm open to any suggestion. To me, it works as long as it's a sane default that's the same for all similar methods. It can be [] false nil

(As long as they are returned differently than a real result could be (ex: YAML null shouldn't return as nil if the default is nil for no result.
We can deal with this by always returning an array of the results and an empty array to mean "" which would be different than anything else --> fallback OR, perhaps, raise a specific Error and handle it.

Once again, I feel tenderlove and hsbt would have valuable input.

[sorryeh]

Note --> The current (most recent rebase) will fail CI because of the new tests I added. (I was changing the default fallbacks which weren't covered by tests before.)

Will fix them to be in the state of the old behaviour and go ahead with a potential improvement plan?

[tenderlove]

This is looking good to me. Once the build is green I'm 👍 on merge / release.

[sorryeh]

@tenderlove now it's 💚

Thanks! (I'll work on some more improvements irw safe operations, the deprecation of current default fallbacks and some other things I saw in the code that could be cleaned up a bit)