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

Relative exclusive scope should skip containing #2110

Closed
wants to merge 2 commits into from

Conversation

AndreasArvidsson
Copy link
Member

@AndreasArvidsson AndreasArvidsson commented Dec 8, 2023

This frequently bites me. "two states" refers to the if statement and the second print statement, but "next state" refers to the first print statement. This change makes it so it skips containing and refers to the second one instead.

    if ( | true) {
        console.log(1)
    }

    console.log(2)

Checklist

  • I have added tests
  • [-] I have updated the docs and cheatsheet
  • [-] I have not broken the cheatsheet

@pokey
Copy link
Member

pokey commented Dec 8, 2023

I think both today's behaviour and your proposed behaviour are useful. And I think both can be equally surprising. It would be quite surprising to pop out of a "curly" you can't see when you say "next curly"

Why don't we support both as separate modifiers? You can map it to "next", and I'll map it to "skip", eg "skip funk", "skip second funk", etc

If we find the other is more often useful, and less often surprising, we can change default spoken form

Make sense?

@AndreasArvidsson
Copy link
Member Author

AndreasArvidsson commented Dec 8, 2023

I'm fine with having two separate modifiers, but I do think having one canonical way of numbering the scopes and then using the same logic for two scopes and next scope is much easier for the users to grasp. I feel myself that I don't really know what Cursorless will use when I issue these different commands.

Should we make a brand new modifier or just add an additional parameter to the modifier? We could also make it a private setting and try it out?

@AndreasArvidsson
Copy link
Member Author

I think there may also be something interaction with interior we haven't experimented with yet. Curly has an interior that may change the behavior compared to statements.

@pokey
Copy link
Member

pokey commented Dec 12, 2023

Should we make a brand new modifier or just add an additional parameter to the modifier? We could also make it a private setting and try it out?

yes I'd make it private. Not sure bout new modifier vs param. One thing to keep in mind: the impl is very very close to RelativeInclusive, so I would be inclined to fold it in there rather than trying to handle both in RelativeExclusive. Or make a separate class. But I don't think it belongs in RelativeExclusive

@pokey pokey added the to discuss Plan to discuss at meet-up label Dec 12, 2023
@AndreasArvidsson
Copy link
Member Author

Should we make a brand new modifier or just add an additional parameter to the modifier? We could also make it a private setting and try it out?

yes I'd make it private. Not sure bout new modifier vs param. One thing to keep in mind: the impl is very very close to RelativeInclusive, so I would be inclined to fold it in there rather than trying to handle both in RelativeExclusive. Or make a separate class. But I don't think it belongs in RelativeExclusive

Implementation wise it might be that, but its still most definitely relative exclusive since we have offset 1?

@pokey
Copy link
Member

pokey commented Dec 12, 2023

Implementation wise it might be that, but its still most definitely relative exclusive since we have offset 1?

yes but keeping it as part of the same impl might be beneficial as it ensures the semantics is exactly the same

@AndreasArvidsson
Copy link
Member Author

We can always make utility function for that part

@AndreasArvidsson
Copy link
Member Author

Closed in favor of #2133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to discuss Plan to discuss at meet-up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants