Skip to content

Conversation

@nacezavrtanik
Copy link
Collaborator

Implemented and documented a highlighting feature.

Resolves #15

if highlight is True:
highlight = default_highlight
if isinstance(highlight, str):
highlight = COLORS_TO_ANSI[highlight.lower()], ANSI_RESET
Copy link
Owner

Choose a reason for hiding this comment

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

This would throw a KeyError if the color is not listed in the COLORS_TO_ANSI dict. Is that expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It kinda is. I did intentionally avoid any explicit error handling, since I figured I'd leave it to the user to respect the type hints and docstrings/docs, and have them be responsible should they use the function in an unintended way. I felt like there were several ways to go when it comes to error handling, and since I wasn't sure where you stand on this, I wanted to keep the changes as minimal as possible.

I see why you would not want to let a KeyError be raised here. Two solutions come to mind:

  1. Check for the inappropriate string value and raise a ValueError instead of a KeyError.
  2. Create a Highlight (str)enum which users may opt to use instead of raw strings, and use that in the annotation.

In case 1., where do we put the check? If we want to preserve the majority of the current implementation, (a) right before accessing the dict would make the most sense. This is a nice mid-ground solution. But since we are now doing error handling anyway, we might as well do it (b) at the beginning of fuzzyfinder, to exit immediately, and perform explicit type checks as well, for consistent exit behavior. But then, do we add explicit checks of other args as well?

Case 2. I feel adds some complexity (or perhaps simplification?) on the user's end, but it does go hand in hand with the approach to error handling I described above, if we would want to stick with that.

I'm fine with any of these tbh. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine if this is intended. I know the available keys are in the docs but people often don't read them all. What if we simply catch the keyerror and print the available keys that they can use and exit?

@amjith amjith merged commit 2587072 into amjith:main Apr 11, 2025
6 checks passed
@nacezavrtanik nacezavrtanik deleted the feature/highlight branch April 11, 2025 12:21
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.

Highlighting feature

2 participants