-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Set prompt color #13
Closed
Closed
Set prompt color #13
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it is not preferred to depend on a specific library in the aspect of backward-compatibility. We cannot change
termbox
to another one because it breaks go-fuzzyfinder's backward-compatibility.Could you define own
Color
type and each color?For example:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think I agree. Because go-fuzzyfinder passes through the defined color code, redefining iotas like this would still involve breaking backwards-compatibility if the termbox dep was replaced later (because there’s no guarantee that the new library defines constant uint16 color codes the same way).
If the goal is to avoid locking the public go-fuzzyfinder API to a specific backing terminal library, I think the code would need to translate everywhere colors are used, so that fuzzyfinder.ColorBlack is converted to termbox.ColorBlack when used (and the same for the other colors), so that it doesn’t rely on the iota layout being permanently consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least now the compatibility between I suggested enums and
termbox
's one will be kept as long as we use Go modules. And iftermbox
breaksColor
backward-compatibility in a new version, we can remap enums we defined when updatinggo.mod
.However, this opinion is definitely correct. Also, we may replace
termbox
totcell
in the near future becausensf/go-termbox
is not now maintained.So, could you remap defined colors to
termbox
's one?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martinbaillie How about you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on this either way. I'm still making use of this library internally at work for some CLIs and flexible prompt colour would be nice.