-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat: improve colored output #26
base: main
Are you sure you want to change the base?
Conversation
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.
This generally looks good to me. I've made a few comments about using builtin helper functions instead of custom ones and some other suggestions/questions
@@ -1,7 +1,93 @@ | |||
local termcolors = { |
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.
Should these come from a user's config, but default to these values to start, so they can be easily overridden?
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.
Yes. What do you think would be the best behavior here;
- if a user specifies a custom pattern, replace all patterns with the users' patterns
- if a user specifies a custom pattern, add on to default patterns, and overwrite the default pattern if they have the same name
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.
@Davincible I think in most cases plugin authors use vim.tbl_deep_extend
i.e. we only ever replace something that has been specifically changed by the user so essentially option 2
Changed to built-in functions. Didn't know about them |
There are two small bugs I'm not sure how to resolve;
Edit: |
Continuation of #24