Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug with interactive.accepts() #39

Open
thekevinscott opened this issue Feb 18, 2024 · 5 comments
Open

Bug with interactive.accepts() #39

thekevinscott opened this issue Feb 18, 2024 · 5 comments

Comments

@thekevinscott
Copy link
Contributor

I believe there is a bug with the accepts() method when using interactive.

I set up a repo here demonstrating the issue.

This is the error:

Screenshot 2024-02-18 at 2 40 06 PM

I believe what is happening is that accepts is attempting to iterate with an of statement here, but it gets back an object from choices() which cannot be iterated over with of (it could be iterated over with in, which would return the object's keys).

@thekevinscott
Copy link
Contributor Author

Just to provide a solution, rewriting the accepts() method to be:

  accepts() {
    let new_cursor;
    let accepts = new Set();
    const choices = this.choices();
    for (const key in choices) {
      const t = choices[key];

Seems to resolve the bug for me.

I see this method defined here and would be happy to push a PR modifying but I'm not familiar with how this library works, so lmk if you'd like help how best to help you.

@erezsh
Copy link
Member

erezsh commented Feb 19, 2024

If you submit a PR with a validating test, I will merge it in.

If your offer of help extends beyond this PR, there are plenty of other tasks that need doing :)

thekevinscott added a commit to thekevinscott/Lark.js that referenced this issue Feb 19, 2024
@thekevinscott
Copy link
Contributor Author

thekevinscott commented Feb 19, 2024

If you submit a PR with a validating test, I will merge it in.

Added here.

Tests pass, but I don't see any tests around InteractiveParser (so I don't think this change is covered by existing tests).

I can look into adding new tests for this, though if so I'll focus on this specific use case (since I'm not very familiar with the existing functionality set).

If your offer of help extends beyond this PR, there are plenty of other tasks that need doing :)

Maybe! Let's start here and see how it goes :D

@erezsh
Copy link
Member

erezsh commented Feb 19, 2024

Sure, if you can, just add a test for this specific case. Thanks!

@thekevinscott
Copy link
Contributor Author

Test added.

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

No branches or pull requests

2 participants