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

Adding a 'select_for_update' wrapper around 'select' method #5

Closed
wants to merge 1 commit into from

Conversation

perljedi
Copy link

@perljedi perljedi commented May 6, 2015

Just adding a simple wrapper around "select" to append "FOR UPDATE" to the end of the resulting statement.

@ilmari
Copy link
Member

ilmari commented May 7, 2015

This is rather inflexible, since there exist other lock modes, e.g. FOR SHARE. In DBIx::Class there's the for resultset attribute, but SQL::Abstract doesn't have a general way of passing options to the methods.

@perljedi
Copy link
Author

perljedi commented May 7, 2015

I'm not sure I understand what you mean by "rather inflexible" ... it has a single purpose, and accomplishes that purpose in the simplest way possible. Unless you aim is to avoid method bloat by doing something like "select_and_lock" providing an argument to the method to choose the locking mechanism. Honestly I think that having "select_for_update" and "select_for_share" as two methods would make for more self explanatory and easier to maintain code.

As for DBIx:Class, I am not using that in my currently application for a host of reasons, so given the choice between adding functionality to SQL::Abstract, or hacking in the $sql .= " for update"; in the client code, the former seemed like the better option.

@ribasushi
Copy link
Contributor

Thanks for the contribution! It however doesn't fit with the "mission statement" of the module.

... choice between adding functionality to SQL::Abstract, or hacking in the $sql .= " for update"...

There is a 3rd option - create a subclass of SQL::Abstract and use that. The overriding philosophy isn't even "avoid method bloat", but "remain as generic as possible" in the style of "do one thing and do it well". Also keep in mind that "...FOR UPDATE" is not even portable syntax.

I am not able to accept this patch nor a variation of one.

@ribasushi
Copy link
Contributor

@perljedi Also note that there already exists a subclass implementing the thing you are after: https://metacpan.org/pod/SQL::Abstract::More#for-clause

@ribasushi ribasushi closed this May 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants