The symbolize_names keyword argument breaks Psych.load / API conflicts

[stomar]

IMO this should be considered before the new option is established in the 3.0 major release.

Current call-seq of Psych.load:

def self.load yaml, filename = nil, fallback = false, symbolize_names: false
  #...
end

The mixing of optional arguments and optional keyword argument does not play well together: fallback can not be set to a hash (the specifically wished use case when the option was added):

Psych.load("", nil, {})  # => false

The provided empty hash is ignored as fallback return value, and the default false is returned instead.

The hash is only used when the symbolize_names keyword argument is also provided. (However, currently this will cause an exception due to an implementation bug in the handling of the fallback argument; a fix is provided in #339.)

Possible solutions to this problem:

• make (the newly added) symbolize_names an optional positional argument (not very elegant...)
• change all optional arguments to keyword arguments (breaking backwards compatibility...)
• change only the fallback argument to a keyword argument (fallback: false); in this case it would be necessary to check whether this might cause similar problems in other methods that already have the fallback option or should have it (e.g. safe_load would be a candidate for also having this option)
• ... ?

[stomar]

See discussions in #337, #339.

[stomar]

Another angle to the current situation:

The API has (accidentally) started to evolve in such a way that various methods that do essentially the same thing (viz. reading yaml) do not provide the same set of options (although these options make sense for all of them). And the crucial problem is that it will not be possible to implement those options for all methods without API changes.

That‘s not a good basis for going into a new major release, IMO.

E.g. load has the symbolize_names option while the fallback option is broken and cannot be fixed, load_file has the fallback option but the symbolize_names option cannot be added. And that‘s for purely „historical“ reasons because the respective PR submitters had interest in only one of the methods.

[stomar]

@hsbt @tenderlove

Any thought on this and related discussions?

[stomar]

I assume this issue will not be consdered for Psych 3.0 / Ruby 2.5...?

[hsbt]

I'm ok with current API implementation. Because Hash and keyword argument will be changed/broke Ruby 2.6 or 3.0. see. https://bugs.ruby-lang.org/issues/14183

We should consider again at Ruby 2.6/3.0 for psych-4.0.0

[k0kubun]

The API has (accidentally) started to evolve in such a way that various methods that do essentially the same thing (viz. reading yaml) do not provide the same set of options (although these options make sense for all of them). And the crucial problem is that it will not be possible to implement those options for all methods without API changes.

I still don't get your point and what you want to fix. We can't discuss in a reasonable way until I understand them.

Some assumptions in my mind to confirm what you think:

• You want to pass {} as Psych.load's fallback argument instead of Psych::FALLBACK.new({}).
• So you want to change the current behavior of Psych.load's fallback argument (that is not internally wrapped with Psych::FALLBACK).
  • This should be a truth because you sent this PR: Fix fallback argument for Psych.load #339
• But you think having Add :symbolize_names option to .safe_load too #337 made it impossible to pass {} as fallback argument without breaking change.
  • I'm guessing that this is what is meant in "it will not be possible to implement those options for all methods without API changes"

---

Given that assumptions, here are my comments for your points:

• The comment from @hsbt states the final solution. When Feature#14183 comes, you can safely pass {} as fallback even with symbolize_names: option. So moving toward that way (having new options as keyword arguments) is fair enough.
• Accepting {} instead of Psych::FALLBACK.new({}) would be a breaking change even without symbolize_names:.
  • So if we really want to have {} as fallback, thinking about how to avoid a breaking change would be a waste of time because it's impossible. Let's think how to make a breaking change.
  • I agree the option "change all optional arguments to keyword arguments (breaking backwards compatibility...)" like Convert fallback option to a keyword argument #342 because users can know the third argument is gone and can immediately fix it to be a keyword argument.
    • This will be good for the long term direction of having Feature#14183. Changing everything to keyword argument is what we should move to, if there's a problem with current interface.

[stomar]

@k0kubun Yes, I think I can agree with these assumptions; but I still don't get what's so difficult to understand.

Are you really implicating that this kind of API is by intention (rather than a pure oversight):

Psych.load_file(f, {})
# but:
Psych.load("", "filename", Psych::FALLBACK.new({}))

Note that it was never mentioned in the relevant issues/PRs, nor added as a test case, nor documented. Also, load is not meant for internal usage only, yet Psych::FALLBACK is clearly an implementation detail, explicitly tagged with nodoc.

The intended behaviour most certainly was (and I really cannot imagine anyone would argue against this):

Psych.load_file(f, {})
Psych.load("", "filename", {})

Furthermore, it should be possible to provide both the fallback and the symbolize_names option (and possible other options) for all the various methods where they would make sense; the fact that one was added to load and the other to load_file was solely by chance, because the PR authors had a use case for only one of those methods.

If you want to call fixing an undocumented, strange behaviour a breaking change rather than a bug fix, well, let's do that.

Personally, I would love to have all those positional optional arguments converted to keyword arguments; but I can not at all judge what the impact of such a change would be and whether it's advisable.

[k0kubun]

Yes, I think I can agree with these assumptions
Note that it was never mentioned in the relevant issues/PRs, nor added as a test case, nor documented. Also, load is not meant for internal usage only, yet Psych::FALLBACK is clearly an implementation detail, explicitly tagged with nodoc.

Okay, now it seems that I understand what you've thought.

Personally, I would love to have all those positional optional arguments converted to keyword arguments; but I can not at all judge what the impact of such a change would be and whether it's advisable.

Yes, and so this kind of thing depends on maintainers' decision. I leave it for them.

[stomar]

Solved by #342.