[MODULES-2462] Improve parseyaml function

[dmitryilyin]

• Add default value support
  Second argument will be returned if yaml cannot be parsed
  instead of false value
• Update tests

[DavidS]

This is a continuation of #509. Please use "git push -f" to the PR's branch to keep the discussion in one place. We can continue here.

Having a default value in case of parse errors is indeed something pick* cannot provide. parseyaml("---\n*8", 'default value') will still error out, as there is no exception handling for the call in the function. I guess that's not what you intended. Add this spec:

it { is_expected.to run.with_params("---\n*8", 'default_value').and_return('default_value') }

to see this.

[dmitryilyin]

Updated the function and spec. Also added same changes for parsejson function.

[holser]

+1

[degorenko]

+1

[bmjen]

👍 Thanks for the contribution and updates @dmitryilyin !

[ffrank]

Hey, er, I realize this is merged by now. But this is a problem.

Don't ever rescue Exception.

Thoughts? This seems to be a recurring issue in stdlib, yes? Should I open a ticket for this?

[ripienaar]

setup rubocop, profit :P

[DavidS]

both, please. weeps

[igalic]

mumbles something about modulesync

[dmitryilyin]

In this case rescuing exception is appropriate because I really want to rescue every possible situation and return the default value. But I guess I can gather a list of exceptions the parser can raise and rescue only them.

[ripienaar]

@dmitryilyin no you really cannot handle every possible exception. You wont handle NoMemoryError, SignalException, fatal, and probably shouldnt mask things like SyntaxError from the user

[dmitryilyin]

Hmm, looks like there is quite a number of possible exceptions that can be raised by YAML module. There is a set of Psych exceptions, there can be Syck exceptions, there are C implementation of YAML loader and a number of Type errors and even Standard errors.
I don't this we really need all these listed here, and, to make things worse, more implementations and exceptions can be added without us knowing it.

So, because we are working with external module without a constant set of possible raised exceptions and we do want to rescue as much exceptions as possible, including Interrupt, Syntax error and even NoMemory error, it's appropriate to use anti-pattern of rescuing all exceptions here.

[ffrank]

FWIW I opened #586 to track this.

The plethora of exceptions from (third party?) modules should inherit StandardError rather than plain Exception. If they do not, then this is either the module author's fault, or there is a good reason for making those hard to catch.

Also no, why would we want to rescue things like Interrupt? If I hit Ctrl-C at the wrong time, the process continues but uses a default value instead of some YAML data, in a way that does not leave so much as a log trace? That's bad UX, and a bug report waiting to happen.