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

Added exception handling for OSError that may be thrown by os.read #5970

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

faker-king
Copy link

@faker-king faker-king commented Jun 21, 2024

Added exception handling for OSError that may be thrown by os.read

@faker-king faker-king force-pushed the master branch 7 times, most recently from 4fff7a5 to 9806712 Compare June 21, 2024 13:37
@faker-king
Copy link
Author

hi @richtja, Can you help me see if I can merge the code

@richtja richtja self-requested a review June 26, 2024 09:34
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 @faker-king, thanks for your contribution. Even though your change is correct, I am not sure about it. IMO, it decreases readability of the code without any additional benefits, but maybe I am missing something here. Can you please help me to understand how your change is avoiding triggering errors? Thank you.

@faker-king faker-king changed the title By optimizing if, else, and using the get method, we can avoid trigge… Added exception handling for OSError that may be thrown by os.read Jun 26, 2024
@faker-king
Copy link
Author

Hi @richtja, Thank you for your feedback. I have carefully considered it and there is indeed no problem with what you said. It has reduced the readability of the code. I have now made a new modification and would appreciate it if you could double check it.

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 @faker-king this change LGTM. We are right now in feature freeze state and we are doing pre-release testing, therefore this PR will be merged after the release planed on Friday June 28.

@faker-king
Copy link
Author

Hi @richtja, I have found that the error in the codeclimate was not caused by my modifications. Could you please help me identify the reason for this

@richtja
Copy link
Contributor

richtja commented Jul 8, 2024

Hi @richtja, I have found that the error in the codeclimate was not caused by my modifications. Could you please help me identify the reason for this

Hi @faker-king, don't worry about the codeclimate error. We are facing some issues with codeclimate these days.

@richtja richtja merged commit e236145 into avocado-framework:master Jul 9, 2024
56 of 57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants