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

Merge two loops #5923

Closed
wants to merge 0 commits into from
Closed

Conversation

changxuqing
Copy link
Contributor

@changxuqing changxuqing commented Apr 30, 2024

The operations of converting characters to ASCII values and generating a list of bits can be combined into one loop, reducing the number of passes.

@changxuqing changxuqing changed the title Merge two loops: The operations of converting characters Merge two loops Apr 30, 2024
@richtja richtja self-requested a review May 15, 2024 13:21
@changxuqing
Copy link
Contributor Author

Hi @richtja ,Could you please help me check the pr?

Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

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

Hi @changxuqing, sorry for the late reply. Thank you for your changes it LGTM. I have just comments to the commits format.

Also, I saw that we lack the test coverage for this method. Since you are touching this. Would you be so kind and write a unit test for it in unit/utils/astring.py. Thank you.

@@ -147,7 +140,7 @@ def strip_console_codes(output, custom_codes=None):
if special_code == tmp_word:
continue
old_word = tmp_word
return_str += tmp_word[len(special_code) :]
return_str += tmp_word[len(special_code):]
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUIC this shouldn't be part of this commit, am I right?

@@ -140,7 +140,7 @@ def strip_console_codes(output, custom_codes=None):
if special_code == tmp_word:
continue
old_word = tmp_word
return_str += tmp_word[len(special_code):]
return_str += tmp_word[len(special_code) :]
Copy link
Contributor

Choose a reason for hiding this comment

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

The commit message, doesn't correspond to the changes. Can you please update the commit message?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done 106
Development

Successfully merging this pull request may close these issues.

None yet

2 participants