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

Allow Log in Safe methods #2783

Closed
wants to merge 1 commit into from
Closed

Allow Log in Safe methods #2783

wants to merge 1 commit into from

Conversation

shargon
Copy link
Member

@shargon shargon commented Jun 28, 2022

Close #2782

@shargon shargon requested a review from erikzhang June 28, 2022 14:36
@shargon shargon marked this pull request as ready for review June 28, 2022 14:50
@roman-khimov
Copy link
Contributor

From a purist POV safe methods are supposed to be free of any side-effects and logging is a side-effect. But practically #2782 is a problem and I don't see how this could be misused, so probably it's OK to allow logging in safe methods.

@erikzhang
Copy link
Member

From a purist POV safe methods are supposed to be free of any side-effects and logging is a side-effect.

I agree.

@shargon
Copy link
Member Author

shargon commented Jun 30, 2022

Log's side-effects depend on the plugin that uses it, because the core itself doesn't do it, so it's safe, until it stops being safe for a third party, which would be the third party's problem, I understand what you're saying but the the problem with not changing it is making error handling in Safe methods unusable.

Any alternative?

@erikzhang
Copy link
Member

Any alternative?

throw.

@shargon shargon closed this Jun 30, 2022
@shargon shargon deleted the safe-log branch June 30, 2022 12:53
@shargon
Copy link
Member Author

shargon commented Jun 30, 2022

But Throw it's catcheable, different than Assert

@erikzhang
Copy link
Member

But Throw it's catcheable, different than Assert

Safe method has no side effects, so it doesn't matter if it is caught, right?

@shargon
Copy link
Member Author

shargon commented Jul 2, 2022

But Throw it's catcheable, different than Assert

Safe method has no side effects, so it doesn't matter if it is caught, right?

It matter because the developer expect to end the execution, so undesired results may happen

@erikzhang
Copy link
Member

It matter because the developer expect to end the execution, so undesired results may happen

It's a safe method. It doesn't matter if the execution is ended or not.

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.

Assert and Safe methods
3 participants