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.

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

Jump to bottom

use the error handler replace to allow non-utf8 to be decoded #381

Merged
merged 4 commits into from Mar 23, 2023
Merged

use the error handler replace to allow non-utf8 to be decoded #381

merged 4 commits into from Mar 23, 2023

Conversation

ejalaa12
Copy link
Contributor

Fixes #379

Signed-off-by: Alaa ejalaa12@gmail.com

@ejalaa12
Copy link
Contributor Author

The available error handler when using the decode method are described here.
I've chosen the surrogateescape because :

'surrogateescape' On decoding, replace byte with individual surrogate code ranging from U+DC80 to U+DCFF. This code will then be turned back into the same byte when the 'surrogateescape' error handler is used when encoding the data. (See PEP 383 for more.)

@ejalaa12 ejalaa12 marked this pull request as ready for review March 23, 2022 09:20
@ejalaa12
Copy link
Contributor Author

ejalaa12 commented Mar 23, 2022 •
edited

While the surrogateescape preserve the byte between decoding/encoding, it's main issue is that decoded_line cannot be printed.

So this print will fail.

So I have two solutions:

  • add a printable_line variable such as: printable_line = line.decode(encoding, errors='replace') and print it instead of decoded_line
  • use the 'replace' handler instead of 'surrogateescape'

@clalancette what are you thoughts on this ?

@clalancette clalancette requested a review from hidmic April 21, 2022 20:29
hidmic
hidmic requested changes Apr 25, 2022
View reviewed changes
ament_cmake_test/ament_cmake_test/__init__.py Outdated Show resolved Hide resolved
ament_cmake_test/ament_cmake_test/__init__.py Outdated Show resolved Hide resolved
@ejalaa12 ejalaa12 requested a review from hidmic April 29, 2022 07:58
@ejalaa12
Copy link
Contributor Author

@hidmic any update on this?

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:14
@ejalaa12
Copy link
Contributor Author

ejalaa12 commented Jul 1, 2022

@audrow @hidmic Is there any update on this ? I made the requested changes already

@ejalaa12
Copy link
Contributor Author

ejalaa12 commented Mar 8, 2023

@hidmic Friendly ping?

hidmic
hidmic approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

@ejalaa12 it's been a minute. It looks like this fell through the cracks several times.

@audrow @clalancette if you are on-board, I think this is a net improvement.

ejalaa12 and others added 3 commits March 14, 2023 13:15
@ejalaa12 @clalancette
use the error handler surrogateescape to allow non-utf8 to be decoded
6bc7e69 
Fixes ament#379

Signed-off-by: Alaa <ejalaa12@gmail.com>
@ejalaa12 @clalancette
use replace as fallback when all encodings failed
628b86e 
Signed-off-by: Alaa <ejalaa12@gmail.com>
@clalancette
Add in a test for invalid characters in ament_cmake_test.
f823d51 
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
clalancette
clalancette approved these changes Mar 14, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I'm sorry for the very long wait here; I finally got a chance to look at this. Besides the original change, I also added a test that test out this functionality. I had to add it into another package, but with that done this checks to make sure that this change works. So with that, I'm happy with this. I'm going to run CI on it next.

With that said, I've done enough work here that I'd like one more opinion before we commit. @mjeronimo can you please take a look and let me know what you think?

@clalancette clalancette changed the title use the error handler surrogateescape to allow non-utf8 to be decoded use the error handler replace to allow non-utf8 to be decoded Mar 14, 2023
@clalancette
Copy link
Contributor

clalancette commented Mar 14, 2023 •
edited

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette @mjcarroll
Update ament_cmake_pytest/test/test_pyunicode.py
f9e3d69 
Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Co-authored-by: Michael Carroll <carroll.michael@gmail.com>
@clalancette
Copy link
Contributor

CI (just up to ament_cmake_test and ament_cmake_pytest):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit e355e9c into ament:rolling Mar 23, 2023
@ejalaa12 ejalaa12 deleted the ejalaa12/issue379 branch March 28, 2023 14:08
This was referenced Apr 12, 2023
Merged
Merged
ejalaa12 added a commit to ejalaa12/ament_cmake that referenced this pull request Nov 6, 2023
@ejalaa12
backport merge ament#381 to humble
6e8bf07 
Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
@ejalaa12 ejalaa12 mentioned this pull request Nov 6, 2023
Merged
clalancette pushed a commit that referenced this pull request Nov 10, 2023
@ejalaa12
backport merge #381 to humble (#490)
ca544b2 
* backport merge #381 to humble

* ament_cmake_pytest needs a buildtool_depend on ament_cmake_test. (#439)

* Add missing buildtool_depend on python3-pytest (#440)

* Fix test skipping logic for missing pytest module (#441)

Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Scott K Logan <logans@cottsay.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@mjcarroll mjcarroll mjcarroll approved these changes

@hidmic hidmic hidmic approved these changes

@mjeronimo mjeronimo Awaiting requested review from mjeronimo

Assignees

@hidmic hidmic

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

non utf-8 stdout output of unit test should not make colcon test fail
4 participants
@ejalaa12 @clalancette @mjcarroll @hidmic