Skip to content

Conversation

@jpenilla
Copy link
Member

@jpenilla jpenilla commented Apr 17, 2024

2024-04-16_22-50-02.mp4

the second coordinate isn't getting suggested, initial investigation shows the issue might be in cloud-core

@jpenilla jpenilla force-pushed the location-suggestion-bug branch from 5fb48e0 to 6e0bbaf Compare May 3, 2024 18:53
@TonytheMacaroni
Copy link

TonytheMacaroni commented May 12, 2024

From what I could find, I believe the issue lies with this portion of CommandFlagParser, as well as this part of CommandTree.

Currently, it's moving the input cursor to before the last token, if the input is empty. This seems to not factor in properly the case where a flag is found. In that case, it should move the input cursor to before the next token after the flag. The highlighted code in CommandTree would also need adjustment to account for this. In its current state, it's skipping the tokens proceeding the last one that may be a part of the value of the flag.

I believe this also affects AggregateParser. In addition, the suggestions for these tests appear to be wrong. They should include the proceeding 22 - so they'd be suggestionList("22 0", "22 1", "22 2", "22 3", "22 4", "22 5", "22 6", "22 7", "22 8", "22 9" and suggestionList("22 1", "22 10", "22 11", "22 12", "22 13", "22 14", "22 15", "22 16", "22 17", "22 18", "22 19"), respectively.

I tried adding an additional test for the aggregate parser, using two different parser types:

// CommandSuggestionsTest#setupEach
this.manager.command(manager.commandBuilder("flags4")
        .flag(manager.flagBuilder("compound")
                .withComponent(
                        AggregateParser.pairBuilder(
                                "x", integerParser(),
                                "y", booleanParser()
                        ).build()
                )
        ));

// CommandSuggestionsTest#testCompoundFlags
final String input11 = "flags4 --compound 22 ";
final List<? extends Suggestion> suggestions11 = this.manager.suggestionFactory().suggestImmediately(new TestCommandSender(), input11).list();
Assertions.assertEquals(suggestionList("22 true", "22 false"), suggestions11);

That test currently fails with:

org.opentest4j.AssertionFailedError: expected: <[22 true, 22 false]> but was: <[0, 1, 2, 3, 4, 5, 6, 7, 8, 9]>

This seems to be a result of skipping the 22 part after parsing the flag, as it's trying suggest for the <x> component, rather than the <y> component. It's similarly also missing the 22 portion in the suggestions.

@jpenilla jpenilla force-pushed the location-suggestion-bug branch from 6e0bbaf to 6b73a22 Compare May 13, 2024 04:38
@jpenilla jpenilla force-pushed the location-suggestion-bug branch from 6b73a22 to 3d0d324 Compare May 13, 2024 20:48
@jpenilla jpenilla marked this pull request as ready for review May 13, 2024 20:50
@jpenilla jpenilla force-pushed the location-suggestion-bug branch from 30fa852 to 66d2a10 Compare May 13, 2024 20:52
@jpenilla jpenilla changed the title Add failing test for location flag suggestions Clean up location suggestions and add more tests May 13, 2024
@jpenilla jpenilla changed the title Clean up location suggestions and add more tests Clean up location parser & suggestions, add more tests May 13, 2024
@jpenilla jpenilla merged commit ed3965c into master May 13, 2024
@jpenilla jpenilla deleted the location-suggestion-bug branch May 13, 2024 20:54
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.

3 participants