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

Raise dedicated exception for "Multiple rows returned" #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gst
Copy link

@gst gst commented Mar 14, 2016

No description provided.

@gst
Copy link
Author

gst commented Mar 14, 2016

This is so to allow to except exactly on that condition/exception..

instead of having to do:

try:
    result = db.get("SELECT blablabla...")
except Exception as err:
    if "Multiple rows returned" in str(err):
        # more than one row returned
    else:
        # "real" exception

@gst
Copy link
Author

gst commented Mar 14, 2016

Had not seen about the project being not actively maintained.. will think about it...

@bazzilic
Copy link

Hi,

this is from the readme for this repo:

This package is not actively maintained, and since it exists primarily for backwards compatibility with the module provided in Tornado prior to version 3.0, I do not intend to make major changes or merge pull requests that do so. You are welcome to create a fork, but pull requests are unlikely to get merged into this repo.

@gst
Copy link
Author

gst commented Mar 16, 2016

@bazzilic

yeah I've seen that short after having submitted my PR..

and I'm considering upgrading my code to sqlalchemy because of that..

you can if you wish close this PR then.

@bazzilic
Copy link

I am not an admin of this repo, just subscribed to it for some reason )

@bdarnell
Copy link
Owner

What would you want to do differently when handling this exception compared to a "real" exception? In my view the get() method is for queries that can never have multiple results (because they use a unique index, LIMIT 1 or an aggregate function). If you're expecting this to be true and it's not, what can you do besides let the error escape? If you are explicitly preparing for the possibility of multiple results then query() seems like the more appropriate method to use.

@gst
Copy link
Author

gst commented Mar 18, 2016

@bdarnell

I agree that the torndb get() method should only be used for queries that can never return multiple results (as you say either by primary key or by a unique index for instance or aggregate queries also effectively) but it's theoretically.. there are situation where you're not sure about that and if you wish to correctly detect the "more than 1 result condition" then it's the thing to do IMHO.. it's exactly the same than Django MultipleObjectsReturned exception : https://docs.djangoproject.com/en/1.9/ref/exceptions/#multipleobjectsreturned.

@bdarnell
Copy link
Owner

bdarnell commented Apr 3, 2016

Yes, this error can definitely occur. But why does it need a distinct class instead of a generic Exception with an explanatory message? Why would you want to catch this error and handle it differently from, say, a sql syntax error? What could it have in common with other TornDbExceptions that would make the common base class useful?

@gst
Copy link
Author

gst commented Jan 22, 2023

@bdarnell because this the the only way to be totally sure that the message in the exception comes from your code/raise line/statement (otherwise it could come(be triggered) from anywhere).

Inspecting a text in a message (exception's message are for humans) is not robust against many possible cases.

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.

3 participants