Possible fix for int parsing

[nico202]

Just a way to manage it, probably exists a better way.
With tests

[coveralls]

Coverage decreased (-0.2%) to 74.88% when pulling 03d0034 on nico202:master into b469dad on dcjones:master.

[sglyon]

Will you please rebase this off current master?

[nico202]

Done. I noticed the removal of the two issue tests because of this rebase :)

[kescobo]

I just ran into an issue in scanner.jl where there are two instances of int(string(c)) (ln 1100 and 1108) that are giving me "int not defined" error. Those should probably now be changed to parse(Int, string(c)) right? I can make a separate PR, but they probably fit here too yeah?

[nico202]

Hi, it's after midnight here, I'll look at it tomorrow.
In case, if you don't mind I'll add+stash it to my PR. Else I think you can PR to my branch so that you have credits for the changes.

However, I'll look at it tomorrow. Thanks :)

[kescobo]

@nico202 It's super minor, I don't feel strongly about getting credit. If you don't mind adding it here I have no objection

[sglyon]

Thank you for the implementation here.

I would really like a second opinion from another pseudo maintainer.

Can any of the watchers of this repo provide their take.

Also pinging @albop

[nico202]

bump

[jdlangs]

It's always good to avoid catching errors as part of a program's normal logic and instead use them for actual exceptional situations.

The best options to me seem to be:

• Drop the leading zero octal parsing option and only use the 0oXXX format.
• Introduce a small validation function that returns a bool and decide to return to string immediately if it returns false.

I would vote for the first option since I find the leading zero parsing potentially confusing but that's just my preference. Note that if we don't want to parse a leading zero number as octal, there's no need for the if statements in this function. The parse function itself can handle "0xFF" or "0o777" input.

[nico202]

@jdlangs thanks for your feedback. Dropping the leading 0 does not seem a good idea to me because it's in the specs: http://www.yaml.org/spec/current.html#id2503753.
Unless we want to to yaml-flavor-julia, we need to address it.

[jdlangs]

Ah ok, that does seem to expect it then.

[jdlangs]

Another option is to modify the implicit int resolver to not match to the int tag if it sees "0329".

[kescobo]

@nico202 Really sorry for this languishing. Is this PR still live, or has it been fixed elsewhere?

[ScottPJones]

@nico202 Really sorry for this languishing. Is this PR still live, or has it been fixed elsewhere?

This is still broken (I've found another number parsing issue as well, with using _ as a separator).

[DSGeoff]

The title appears to be referencing issue #41, but I'm not seeing that directly linked anywhere, so doing that now.

I just made some comments in issue #133 (which is linked here) to explain that how this is "fixed" should depend on which version of YAML is implemented in this package, which may not have been clearly mentioned anywhere. If it's 1.2+, I believe octals should require the letter o, e.g., 0o14. And, if that octal change is implemented here, then it might be good for the test to also include an octal. But, that depends on #133.