GroupSeparator gets in the way of parsing lists

[emayevski]

When parsing a construct like "1, 2, 3" and group separators are enabled for numbers, the parser treats a comma as a group separator. Then it does return just a number, but the "separator" itself (not a separator at all) is lost. The problem is in this code of ReadDecimal() in Scanner.cs:

        // Number can be empty if we have a decimal separator directly, in this case don't expect group separators
        if (!number.IsEmpty && allowGroupSeparator && Cursor.Current == groupSeparator)
        {
            // Group separators can be repeated as many times
            while (true)
            {
                if (Cursor.Current == groupSeparator)
                {
                    Cursor.AdvanceNoNewLines(1);
                }
                else
                if (!ReadInteger())
                {
                    break;
                }
            }
        }

The solution is to restore the cursor to point at the first "group separator" as follows:

        // Number can be empty if we have a decimal separator directly, in this case don't expect group separators
        if (!number.IsEmpty && allowGroupSeparator && Cursor.Current == groupSeparator)
        {
            var savedCursor = Cursor.Position;
            // Group separators can be repeated as many times
            while (true)
            {
                if (Cursor.Current == groupSeparator)
                {
                    Cursor.AdvanceNoNewLines(1);
                }
                else
                if (!ReadInteger())
                {
                    // it was not a group separator, really, so go back where the symbol was and stop
                    Cursor.ResetPosition(savedCursor);
                    break;
                }
            }
        }

I will create a separate pull request for this fix later.

[sebastienros]

What I am noticing in the tests is that when 123.e is parsed it fails because it's not a decimal. Now that is opposite to parsing 123,e which would then pass resulting with 123. This is not consistent and that's a problem too. I am not yet sure which one is correct. But since other ReadXYZ method will parse something as long as it matches, it could be that the 123.e test needs to be adapted (following your suggestion).

Independently from that I would suggest you change your grammar to prevent the conflicting grammar. I don't see how users would not be confused by something like (1,2,3). With your suggestion (which I am not refuting) it would return 1,2 and 3. Is that really what you'd want? I would simply disable decimal groups, or change the char to something different, like _, that does not conflict with other parts of the grammar.

[emayevski]

The grammar is defined in and by ncalc, and we cannot change it there. I noticed the problem because if I enable the use of group separators there, some tests related to function parameters and lists start to fail due to the problem I am trying to address.

The idea is that when a user copies some expression, it may contain decimal separators. And if those happen to be commas, either the parser should handle them properly, or the user would have to delete them all by hand. About your example - "1, 2, 3" with spaces should be a list. "1,2,3" without spaces would have to be parsed as a number "123", I guess, as decimal separators are to be skipped (if this is not so now, I would have to do verify my fix more to ensure this is so). Strictly speaking, the parser would have to evaluate how many digits are after each separator, but this would overcomplicate the parser and still be error-prone in the case of say "123,456,789".

[emayevski]

I have added the tests for "123,456" and "123,456,789" to ensure that these are treated as numbers with group separators. So, "1,2,3" without spaces will become a number 123, not 1 or 12, and this is logical.