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

fix(commands): fix an off-by-one in toggle_option #12426

Closed
wants to merge 1 commit into from

Conversation

RoloEdits
Copy link
Contributor

@RoloEdits RoloEdits commented Jan 6, 2025

With the off-by-one, it would work only if the first option was not the same something as what the option was currently set. Now, it properly toggles.

toggle_off_by_one_fix

There is still an existing issue that this way of toggling really only toggles for two options, but more are supposed to be accepted. I would argue though that is actually what "toggle" is supposed to be, not a "cycle".

@the-mikedavis
Copy link
Member

There is still an existing issue that this way of toggling really only toggles for two options, but more are supposed to be accepted. I would argue though that is actually what "toggle" is supposed to be, not a "cycle".

Accepting more than two options was intentional, see #4411

@RoloEdits
Copy link
Contributor Author

RoloEdits commented Jan 6, 2025

I think even with that old way it wasn't sound with duplicate inputs. Is it still worth keeping that spurious behavior for the cases that it works?

@RoloEdits
Copy link
Contributor Author

Should work the same as before. The off-by-one was calling nth(1) instead of next in the unwrap_or_else.

@RoloEdits
Copy link
Contributor Author

Will open up a new one with this fix included in the larger changes.

@RoloEdits RoloEdits closed this Jan 6, 2025
@RoloEdits RoloEdits deleted the toggle-off-by-one branch January 14, 2025 02:55
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