Skip to content

Conversation

@liamwhite
Copy link
Contributor

@liamwhite liamwhite commented Nov 15, 2025

The logic in this function was getting a bit unwieldy, so it's tidied up in this change by separating the logic for what counts as a special char from how to find it. It also unnests the conditionals and removes the need to constantly index into the input array.

}

fn is_special_char(&self, value: u8) -> bool {
if value == b'^' && self.within_brackets {
Copy link
Contributor Author

@liamwhite liamwhite Nov 15, 2025

Choose a reason for hiding this comment

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

If it is possible for this to be incorrect without also testing special_char_bytes, a test case for that would be good

Copy link
Owner

Choose a reason for hiding this comment

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

It'd very be surprising if it could! It would suggest it had to do something with options.parse.smart. I think it's fine to leave as-is.

Copy link
Owner

@kivikakk kivikakk left a comment

Choose a reason for hiding this comment

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

Nice, thank you! I'm going to condense that enumerate/find/map sequence into a position call. I imagine they'll compile to the same machine code either way!

}

fn is_special_char(&self, value: u8) -> bool {
if value == b'^' && self.within_brackets {
Copy link
Owner

Choose a reason for hiding this comment

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

It'd very be surprising if it could! It would suggest it had to do something with options.parse.smart. I think it's fine to leave as-is.

@kivikakk kivikakk enabled auto-merge November 15, 2025 05:28
@kivikakk kivikakk merged commit 099d13b into kivikakk:main Nov 15, 2025
22 checks passed
@liamwhite liamwhite deleted the find-special branch November 15, 2025 05:31
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