-
Notifications
You must be signed in to change notification settings - Fork 63
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 #243 Added module file for text_styling in utils #254
base: master
Are you sure you want to change the base?
Conversation
for i in list(message.split("`")): | ||
|
||
if index % 2: | ||
style_message += style(i, fg="cyan") |
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.
Nice logic here. A small query here: what do you think about adding a check to see if the text is actually enclosed in backticks. That is, to check if there is a closing backtick and only applying this after that.
I'm not sure if it is easy to do here, but if the parts of the messages were in a list ls
:
if len(ls) == ODD:
# apply command style
Let me know what you think.
Another small query: I think we can add bold=True
here?
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.
Ya sure. both of them are doable and looks nice. I will follow it in the following PR
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 I will do that in following PR
style_message += style(i, fg="red", bold=True) | ||
|
||
else: | ||
style_message += style(i, fg="green") |
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.
You used the error
flag here to either print the error message or the success message. But there could be times when we just want to print the info with white bold text or just plain white text. Can you add that functionality here somehow? Feel free to use helper methods as mentioned in #234
Small query: bold=True
?
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.
Ok, I will include it in following PR.
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 just followed the codes of the other files. most of the error messages where bold in those files so I kept it bold.
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 will add an argument to the functions for bold text in the following PR
@ShubhamPandey28 Great work on the issue and kudos on your first PR for CloudCV! 🎉 I have left a few queries, so please have a look. |
Also, please let me know why you chose a single method instead of several helper methods. |
the helper functions will be very similar in that case. But you are right, it will be more easy to maintain with those helper functions. |
Added text_styling.py which solves both issue #234 and #243 at the same time.