fix: Don't parse three consecutive 0's as coordinates [skip ci]

[DonkeyBlaster]

This goes with the assumption that no one will ever type 200,000,000 for the coordinates 200,0,0. Instead of parsing to coordinates, when we see three consecutive zeros, ignore and assume it is a regular number instead.
200,00,00 still is parsed to coordinates since that's not a valid way to write 2 million.

This is the only change I made, if it's easier to review this way

[ShadowCat117]

Looks good. Hopefully less unintentional coordinates parsed now

[magicus]

This does not seem correct. You will fail to parse coordinates that contain 0.

[magicus]

I strongly recommend that you fix this by test-driven development -- that is, start by writing a set of tests of the regexp, both ones that should work right now (incl e.g. 1000,0,0 or 1002,1002), and negative tests the ones with 000 that you would like to not be parsed as coordinates. The latter will fail with the current code. Then you implement your fix, and verify that all tests now pass.

[magicus]

Also, I think you can in any case reduce the number of digits to 5. I don't think Wynn uses any coordinates ouside 99999.

[kristofbolyai]

I strongly recommend that you fix this by test-driven development -- that is, start by writing a set of tests of the regexp, both ones that should work right now (incl e.g. 1000,0,0 or 1002,1002), and negative tests the ones with 000 that you would like to not be parsed as coordinates. The latter will fail with the current code. Then you implement your fix, and verify that all tests now pass.

In fact, Static-Storage has a dump of all coordinates that Wynn quests mention (in the first part of the quest). You could even test against those.

[magicus]

I'm mostly worried about coordinates for quest-specific partial maps. They can be larger than 9999, I think.

[DonkeyBlaster]

Should be fine now. Essentially,

• I reduced the x/z coordinates from {1,6} to {1,5}
• Whitespaces between x/y/z groups are now mandatory, with the \s {0, 2} change to {1, 2}
• The last Z group checks that the number either starts or ends with a non-zero digit, or it's a singular zero

[magicus]

Maybe also add something like:

        p.shouldNotMatch("10591,106,2489");

if we agree that having the whitespace mandatory is a good thing.

[magicus]

This looks generally better, I think. I'm perhaps a bit worried about the requirement of whitespace, but then again, this is only the strict matching. We do have a non-strict version as well, with more flexible parsing abilities, right?

[DonkeyBlaster]

This looks generally better, I think. I'm perhaps a bit worried about the requirement of whitespace, but then again, this is only the strict matching. We do have a non-strict version as well, with more flexible parsing abilities, right?

The whitespace I'm talking about is the [\\s,] which includes commas. This is why I thought it was weird that it was optional to have these, otherwise even 010494 is a "valid" coordinate

Technically it should still match 104,,104,,50 at the moment but I don't think this is too bad?

[magicus]

(?:[\\s,]{1,2} looks just plain wrong. I had not noticed that. sigh Regexes are really write-only code.

It might be time to retire the entire existing expression and create a new one that is more comprehensible and works more properly. Starting by a bunch of examples that should -- and should not -- match the coordinate pattern might be a good way.