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

Update _process_stdout within SlingResource to handle Emojis #27241

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

Conversation

TCronino
Copy link

Added specific error handling for a range of emoji symbols. Relates to issues #27239

Summary & Motivation

When sling stdout sees an emoji, it causes an error as described in issue 27239 . Occasionally sling returns a plug to checkout their side which includes a 👉.

https://github.com/slingdata-io/sling-cli/blob/main/cmd/sling/sling_media.go

How I Tested These Changes

I've not been able to replicate the issue as it seems almost random when it appears....but maybe someone can debug when sling pushes that specific alert...

Changelog

Insert changelog entry or delete this section.

Added specific error handling for a range of emoji symbols
@mlarose
Copy link
Contributor

mlarose commented Jan 21, 2025

I've not been able to replicate the issue as it seems almost random when it appears....but maybe someone can debug when sling pushes that specific alert...

I think creating a unit test that stubs the cli execution and returns an example of a good and bad output and make sure that your changes are producing the expected filtered result in both cases would be a good way to start.

@TCronino
Copy link
Author

Heads up that it shouldn't be an issue following the next Sling release as they will remove the emoji
slingdata-io/sling-cli#490

However, probably still a good issue to solve anyway!

@mlarose
Copy link
Contributor

mlarose commented Jan 22, 2025

Heads up that it shouldn't be an issue following the next Sling release as they will remove the emoji slingdata-io/sling-cli#490

However, probably still a good issue to solve anyway!

Yeah, I don't see a good reason not to future proof from future emoji bombing in the output.

Did you consider adding a test on this?
Also, could you make ruff before resubmitting the PR to make the CI happy?

@TCronino
Copy link
Author

Yup agreed, will circle back to this when I get a chance 👍

@TCronino
Copy link
Author

@mlarose - i have added a test case which looks like it's passing. However i'm a bit of a noob and am not 100% sure the best way to resolve this issue with the ruff check. Any recommendation?

ruff check
test_dagster_sling_translator.py:169:49: SLF001 Private member accessed: `_process_stdout`
    |
168 |     sling_resource = SlingResource(connections=[])
169 |     processed_lines = [line.strip() for line in sling_resource._process_stdout(example_stdout)]

Copy link
Contributor

mlarose commented Jan 23, 2025

Sure. Accessing private members in context of tests is acceptable, so you can ignore it.

Appending this instruction to the lines where you are accessing private members should make ruff happy # noqa: SLF001

@mlarose mlarose self-requested a review January 23, 2025 12:46
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.

2 participants