Skip to content

[rqd] Fix non ASCII chars #1335

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jul 10, 2024
Merged

[rqd] Fix non ASCII chars #1335

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fe618e3
[rqd] Fix non ASCII chars
ramonfigueiredo Jan 17, 2024
8fdfc93
[rqd] Fix non ASCII chars
ramonfigueiredo Jan 17, 2024
20d5864
[rqd] Fix non ASCII chars
ramonfigueiredo Jan 17, 2024
31e1402
[rqd] Fix non ASCII chars
ramonfigueiredo Jan 19, 2024
2e1ece4
Merge branch 'AcademySoftwareFoundation:master' into fix_non_ascii_chars
ramonfigueiredo Jul 3, 2024
75f9236
[rqd] Fix: Ensure consistent handling of non-ASCII characters in logs
ramonfigueiredo Jul 3, 2024
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
2 changes: 1 addition & 1 deletion docs/conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
#
# This is also used if you do content translation via gettext catalogs.
# Usually you set "language" from the command line for these cases.
language = None
language = 'en'

# List of patterns, relative to source directory, that match files and
# directories to ignore when looking for source files.
Expand Down
2 changes: 1 addition & 1 deletion docs/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
sphinx==4.3.1
sphinx==5.0.0
sphinx-rtd-theme==1.0.0
2 changes: 2 additions & 0 deletions rqd/rqd/rqcore.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1219,6 +1219,8 @@ def print_and_flush_ln(fd, last_timestamp):

remainder = lines[-1]
for line in lines[0:-1]:
# Convert to ASCII while discarding characters that can not be encoded
line = line.encode('ascii', 'ignore')
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ramonfigueiredo Thank you for the PR and the detailed write up. The detail makes it nice and easy to review with the proper context.

So, this error occurs when we intercept output in order to prepend a timestamp. What is logged to the file when RQD_PREPEND_TIMESTAMP is False?

My sense is that the output which is logged when RQD_PREPEND_TIMESTAMP is True vs False should be as similar as possible, aside from the timestamp of course.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dear @bcipriano ,

Sorry for the delay in answering your question. Thank you for your thorough review and the positive feedback on the PR and write-up.

Regarding your comment, the UnicodeEncodeError occurs when we intercept output to prepend a timestamp. When RQD_PREPEND_TIMESTAMP is False, the output is logged directly without any modification or encoding to ASCII, preserving the original characters, including any non-ASCII ones.

Here is how the output differs based on the value of RQD_PREPEND_TIMESTAMP:

  1. When RQD_PREPEND_TIMESTAMP is True:
  • We intercept the output to prepend a timestamp.
  • Non-ASCII characters are encoded to ASCII with the 'ignore' option, discarding any non-ASCII characters.
  • Example log: "[12:34:56] text here , caf" (non-ASCII characters are ignored).
  1. When RQD_PREPEND_TIMESTAMP is False:
  • The output is logged directly without any modification.
  • Non-ASCII characters are preserved.
  • Example log: "text here 영화, café" (non-ASCII characters are retained).

To ensure the outputs are as similar as possible, aside from the timestamp, I can adjust our approach to handle encoding more gracefully. Instead of discarding non-ASCII characters, I can consider other strategies, such as escaping them or converting them to a specific placeholder.

However, given the current solution, the primary goal was to prevent crashes due to UnicodeEncodeError by ignoring non-ASCII characters when RQD_PREPEND_TIMESTAMP is True (default option). This approach was chosen for simplicity and robustness.

If maintaining non-ASCII characters is critical, I can explore additional strategies to handle encoding more gracefully without discarding valuable information. I am open to suggestions and further discussion on the best approach to balance robustness and information retention.

Thank you again for your review and insights. I look forward to your feedback.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @bcipriano

FYI ...

The updated logic to ensure consistent handling of non-ASCII characters in logs with RQD_PREPEND_TIMESTAMP = True or RQD_PREPEND_TIMESTAMP = False.
print("[%s] %s" % (curr_line_timestamp, line), file=outfile)
outfile.flush()
os.fsync(outfile)
Expand Down