Skip to content

More specific exception classes - #911

Merged
sodabrew merged 3 commits into
brianmario:masterfrom
Shopify:more-specific-exception-classes
Nov 30, 2017
Merged

More specific exception classes#911
sodabrew merged 3 commits into
brianmario:masterfrom
Shopify:more-specific-exception-classes

Conversation

@casperisfine

Copy link
Copy Markdown
Contributor

Take over: #870
Partially fix: #404

I've just implemented 2 of them to showcase what it could look like. I can go over all error codes if dimmed necessary.

(Sorry for opening a second PR, but long story short I lost access to the first fork I did).

@sodabrew does this look good to you?

cc @rafaelfranca & @sgrif

@sodabrew

Copy link
Copy Markdown
Collaborator

This looks good to me. Will need a rebase since the Ruby 1.9-syntax changes landed in another PR. Could you run down the unit test failures?

@sodabrew sodabrew added this to the 0.5.0 milestone Nov 27, 2017
@casperisfine
casperisfine force-pushed the more-specific-exception-classes branch 2 times, most recently from aece179 to 6c271b5 Compare November 28, 2017 10:01
@casperisfine

Copy link
Copy Markdown
Contributor Author

@sodabrew sorry I should have checked CI after my rebase.

I fixed the test suite. Should be good to go now.

Comment thread lib/mysql2/error.rb
super(clean_message(msg))
end

def self.new_with_args(msg, server_version, error_number, sql_state)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I remember there was a reason for all of this, but I don't remember what that reason was 🤕

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I remember git blaming it and figuring out that it wasn't required anymore. Let me blame again.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hum, don't remember why I dimmed it unnecessary #545

I still think it's better this way, but I can remove that change if you prefer.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I moved the arguments into the constructor in f9eba44 but moved them out to a separate method in 1d4de89

@sodabrew

Copy link
Copy Markdown
Collaborator

Thanks very much for the rebase and updating the unit tests!

@sodabrew

Copy link
Copy Markdown
Collaborator

LGTM. Do you want to wait for feedback from the other folks you tagged in the OP?

@casperisfine
casperisfine force-pushed the more-specific-exception-classes branch from 6c271b5 to eb48219 Compare November 30, 2017 10:12
@casperisfine

Copy link
Copy Markdown
Contributor Author

Do you want to wait for feedback from the other folks you tagged in the OP?

No don't worry, they're coworkers, I'll ping them more directly. And since I had to rebase again, I'd rather get this merged ASAP :p

@sodabrew
sodabrew merged commit 1813043 into brianmario:master Nov 30, 2017
@casperisfine
casperisfine deleted the more-specific-exception-classes branch November 30, 2017 15:15
@rafaelfranca

Copy link
Copy Markdown

Thanks for doing this 👍

jeremy added a commit to jeremy/mysql2 that referenced this pull request Dec 11, 2017
* master:
  GitHub is HTTPS by default (brianmario#922)
  Fix wrong value of type YEAR on big endian environment. (brianmario#921)
  Avoid 0 byte allocation call when statement takes no parameters
  Small spec improvement for RSpec style
  Travis CI for Mac OS X builds use the same steps again
  More specific exception classes (brianmario#911)
  Travis CI matrix add Ruby 2.5 and switch Mac OS X to Ruby 2.3 (ala High Sierra)
  README text for query options on Statement#execute
  Accept query options on Statement#execute (brianmario#912)
  Drop 1.9.3 support (brianmario#913)
kamipo added a commit to kamipo/rails that referenced this pull request Sep 13, 2019
This follows up rails#36692, `Mysql2::Error::TimeoutError` is introduced from
mysql2 gem 0.5.0. brianmario/mysql2#911
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.

Implement more specific exception classes

4 participants