Add :symbolize_names option to .safe_load too

[k0kubun]

suggested at #333 (comment)

Summary

I added the same thing as #333 only for .safe_load.

Notes

For .load_file, as it may take Hash object in existing arguments, we can't add :symbolize_names to it keeping backward compatibility (reason is described below).

---

The reason I didn't add the option to Psych.load_file

Why adding :symbolize_names to Psych.load_file is incompatible?

before

File.write("test.yml", "")
Psych.load_file("test.yml", {}) #=> {}

after

After adding symbolize_names: false to .load_file signature,

File.write("test.yml", "")
Psych.load_file("test.yml", {}) #=> false

{} is considered as empty keyword arguments, and second argument of Psych.load_file becomes false.

Why adding it to Psych.load and Psych.safe_load is safe?

That's because Psych.load and Psych.safe_load don't take Hash object for normal usages.

Unlike Psych.load_file, fallback argument of Psych.load is NOT wrapped with Psych::FALLBACK. Thus, if you pass {} as fallback, {}.to_ruby will be called and it will be broken. So this method can't take {} (We should pass Psych::FALLBACK.new({}) if we want to have {} as fallback).

[hsbt]

Thanks a lot!

The reason I didn't add the option to Psych.load_file

Sounds reasonable.

[stomar]

Indeed causes the mixing of optional positional arguments and optional keyword arguments problems, see #340. I would argue that calling Psych.load with {} as fallback should work similar to Psych.load_file, and consider it a bug that it raises an exception. (It shouldn't make a difference whether the yaml comes from a file or a string.)

[k0kubun]

raises an exception

You mean calling {}.to_ruby? It's not handled explicitly and I don't think it's a kind of feature which was maintained. This PR doesn't break existing working application unlike #339 (comment) and is less harmful. Changing or unifying fallback interface is totally separated issue.

Anyway, my opinion is here #339 (comment)

[stomar]

See also #339.

Short version: load and load_file should work exactly in the same way. load blowing up when called with a fallback is a bug. The way the new option is implemented prevents fixing this bug. Furthermore it doesn't make any sense that load has the new option and load_file has not.

[k0kubun]

Short version: load and load_file should work exactly in the same way.

I agree for that point. Obviously it was already an existing problem (wether it's a bug or not depends on intention of the person who implemented. It's not necessarily a bug if the behavior is intentional), but I admit adding an option to only some of them would make the matters worse.

To be clear, my comment in the PR is NOT against the point. I'm in favor of unifying interfaces, but using keyword arguments for future extensions.

[stomar]

In the issues related to the fallback option (#149, #264) the single point was to provide a possibility to specify a fallback. There was never any hint that load should not accept a fallback parameter and blow up instead. IMO it's unimaginable that anyone would implement that kind of behaviour by intention.

The current situation: someone who wants symbolized keys must use load, someone who wants to have a custom fallback must use load_file. What about someone who needs both? This API is extremely broken. (The fact alone that this option can not be added to some methods because of purely "technical" reasons should indicate that something is wrong.)

I'm all for using keyword arguments, but in this case either the new option needs to be implemented differently, or older behavior changed (fallback as keyword argument, if that doesn't cause problems in other methods). I'd rather prefer the latter option, if it's acceptable to break compatibility.

BTW, this is not about unifying interfaces, it's about keeping the interfaces unified. All those methods (load, load_file, safe_load, ...) once provided the same functionality, but in the meantime the behaviour diverged, because options were added to only some of them. This should be fixed, not increased further.

[k0kubun]

someone who wants to have a custom fallback must use load_file

Why do you think you can't use Psych.load(yaml, nil, Psych::FALLBACK.new({})) ? That's the correct usage from previous versions.

[stomar]

That's certainly not the intended usage:

Psych.load_file("empty.yml", {})
# versus:
Psych.load(yaml, nil, Psych::FALLBACK.new({}))   # ????

That would be crazy. Psych::FALLBACK is an implementation detail and not part of the public API. It's explicitly not documented. Having the library user care about wrapping his fallback value would be the worst interface design ever.

Please have another look at the two fallback issues and the code and evaluate what is more probable: an extremely strange interface relying on the user to care about internal helper structs, a behaviour that was never mentioned or discussed anywhere in the PRs, or a clean, similar interface for the two methods (which essentially do exactly the same thing) and a simple bug, caused by wrapping at the wrong place.