Skip to content

🩹 Fix sequence#1907

Open
SnaveSutit wants to merge 2 commits intoSpyglassMC:mainfrom
SnaveSutit:fix/sequence
Open

🩹 Fix sequence#1907
SnaveSutit wants to merge 2 commits intoSpyglassMC:mainfrom
SnaveSutit:fix/sequence

Conversation

@SnaveSutit
Copy link
Contributor

This change allows the gapParser to be used to show context errors, like "expected space", between arguments, while also allowing for optional arguments in a sequence with such a gapParser.

The current behavior is gap parser will run between every item in the sequence, whether or not those parsers actually moved the cursor or returned nodes. So multiple chained optional arguments are fiddley to implement.

function sequenceDemo(): core.Parser<core.AstNode> {
    // An InfallableParser that produces an error if it encounters a gap that isn't exaclty one space.
    const sep = core.map(mcf.sep, () => [])

    return core.setType(
        'sequence_demo',
        core.sequence([
            core.literal('sequence'),
            // gapParser is called
            core.optional(
                core.failOnEmpty(
                    core.resourceLocation({
                        category: 'tag/function',
                        usageType: 'reference',
                        allowTag: false,
                    }),
                ),
            ),
            // In the current implementation, the gapParser (sep) is called again between these
            // two parsers, whether or not the optional parser actually captured anything.
            // Because the gapParser shows an error if the gap isn't exactly one space,
            // it will always show an error when the optional parser returns `undefined`, as
            // the space has already been consumed by the previous gapParser call.
            core.literal('test'),
        ], sep),
    )
}

To work around this issue with the current parser, we need to put sep between every argument (tedious code duplication), and add an extra core.sequence call inside the optional parser in order to only check for that sep when it's actually needed creating extra tree depth that could be avoided. (adds more indentation levels. 🤢)

function sequenceDemo(): core.Parser<core.AstNode> {
    // An InfallableParser that produces an error if it encounters a gap that isn't exaclty one space.
    const sep = core.map(mcf.sep, () => undefined)

    return core.setType(
        'sequence_demo',
        core.sequence([
            core.literal('sequence'),
            sep,
            core.optional(
                core.sequence([
                    core.failOnEmpty(
                        core.resourceLocation({
                            category: 'tag/function',
                            usageType: 'reference',
                            allowTag: false,
                        }),
                    ),
                    sep,
                ]),
            ),
            core.literal('test'),
        ]),
    )
}

This change allows the `gapParser` to be used to show context errors, like "expected space", between arguments, while also allowing for optional arguments in a sequence with such a `gapParser`.

The current behavior is gap parser will run between every item in the sequence, whether or not those parsers actually moved the cursor or returned nodes. So multiple chained optional arguments are fiddley to implement.
Copy link
Member

@MulverineX MulverineX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these updated snapshots are incorrect results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants