Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ShubhamPandey28
Copy link

Added text_styling.py which solves both issue #234 and #243 at the same time.

for i in list(message.split("`")):

if index % 2:
style_message += style(i, fg="cyan")
Copy link
Contributor

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?

Copy link
Author

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

Copy link
Author

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")
Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Author

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.

Copy link
Author

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

@nikochiko
Copy link
Contributor

@ShubhamPandey28 Great work on the issue and kudos on your first PR for CloudCV! 🎉
I loved how you tried to generalize everything in one function.
The readability of the code is good as well.

I have left a few queries, so please have a look.

@nikochiko
Copy link
Contributor

nikochiko commented Jan 11, 2020

Also, please let me know why you chose a single method instead of several helper methods.
I was thinking of having several helper methods, so we could add them as we like. So, if we use flags as mentioned and the number of states increases, maintaining the code becomes more difficult.

@ShubhamPandey28
Copy link
Author

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.

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.

None yet

2 participants