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

Make exception API compatible with what Ruby expects #42

Merged
merged 1 commit into from
May 20, 2022

Conversation

jeremyevans
Copy link
Contributor

@jeremyevans jeremyevans commented May 16, 2022

Ruby expects that the first argument to Exception#initialize is a
string (or nil), since that is what Kernel#raise uses. Breaking that
assumption is a very bad idea.

This problem was introduced in 16be09a.
A backwards compatible approach would have added a response keyword
argument. Unfortunately, that ship has sailed, unless we want to break
backwards compatibility again.

Try to restore backwards compatibility and align with standard Ruby
exception behavior by checking whether the first message to #initialize
is an STMP::Response. If so, use it as the response. If not, treat it
as the exception message.

This prevents issues in code that does something like:

  raise exception_class, exception_message

Before this change, you'll get a NoMethodError later inside of
SMTPError#message, with no indication of the actual problem.

Ruby expects that the first argument to Exception#initialize is a
string (or nil), since that is what Kernel#raise uses.  Breaking that
assumption is a very bad idea.

This problem was introduced in 16be09a60c77bcf7ce10fa91cc3689c0d11b0f4bm.
A backwards compatible approach would have added a response keyword
argument.  Unfortunately, that ship has sailed, unless we want to break
backwards compatibility again.

Try to restore backwards compatibility and align with standard Ruby
exception behavior by checking whether the first message to #initialize
is an STMP::Response.  If so, use it as the response.  If not, treat it
as the exception message.

This prevents issues in code that does something like:

```ruby
  raise exception_class, exception_message
```

Before this change, you'll get a NoMethodError later inside of
SMTPError#message, with no indication of the actual problem.
@jeremyevans jeremyevans requested review from hsbt and tmtm May 16, 2022 20:27
@bestwebua
Copy link

Good point 🥰 But also this library isn't following to the semvers standards too( Just look to this crazy adapter, which allows to tame minors versions: https://github.com/truemail-rb/truemail/blob/667888cbc4260db5a992a4a3f2612bc2b6882a5e/lib/truemail/validate/smtp/request.rb#L68 😀

@jeremyevans
Copy link
Contributor Author

I think almost every Rubyist expects to be able to use Kernel#raise with two arguments, regardless of which exception class is used. Violating that expectation should not be done, even if doing so is technically considered allowable from a backwards compatibility standpoint due to sub 1.0 versioning.

@bestwebua
Copy link

Of course! Thanks for this catch. My comment was not about yours improvement. It's about this library, in general.

Copy link
Collaborator

@tmtm tmtm left a comment

Choose a reason for hiding this comment

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

👍

@tmtm tmtm merged commit 1d28fe4 into ruby:master May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants