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

format idAttribute on save/delete (#1676) #1680

Merged
merged 4 commits into from Feb 8, 2018

Conversation

Projects
3 participants
@osher
Contributor

osher commented Nov 2, 2017

format idAttribute on save/delete (#1676)

Introduction

Working with a DB and Web that do not agree on field-names conventions - trying to leverage Model#format() and Model#parse() worked for selects and inserts - but fails for udpates and deletes.

Motivation

This PR fixes that. Closes #1676, Closes #1338.

Proposed solution

use Model#format to transform the id attribute name passed to .where in delete and update operations.

Current PR Issues

none

Alternatives considered

none

@rapzo

This comment has been minimized.

Contributor

rapzo commented Nov 2, 2017

I'll give a round on this, although, we could move the format up a bit instead of running it twice, even for a single key, it looks harder to read than it'd be with a single call.

@osher

This comment has been minimized.

Contributor

osher commented Nov 8, 2017

Lost you there, you mean just to do it in two lines instead of one, or create a more reusable method and call it from there?

LMK - I'll comply

@osher

This comment has been minimized.

Contributor

osher commented Nov 8, 2017

P.S - I met more bugs around the same problem which appears in different places around delete and updates, although I'm yet sure if the root cause is in Bookshelf itself, or in bookshelf-modelbase plugin.

I tend to go for creating a method that provides a selector/filter/where clause that should be used wherever a key to a table is needed.

Not just a private function, but a part of the API.
If all implementations will pull table key from this API - it will also make supporting compound keys in the future easy peasy

@osher

This comment has been minimized.

Contributor

osher commented Nov 15, 2017

anything?

@ricardograca

This comment has been minimized.

Member

ricardograca commented Nov 15, 2017

I'm not that sure about increasing our dependency on .format() and .parse(), but I'm not contributing much code to the project anyway, so maybe someone more active can comment on this issue.

@ricardograca ricardograca added this to To Do in Version 0.13.0 Jan 3, 2018

@ricardograca

This is very similar to #1338. Can you explain why your solution is better? Also, can you provide test cases for the issues this fixes?

@osher

This comment has been minimized.

Contributor

osher commented Feb 6, 2018

Yup. looks very similar. I think mine uses less operations, but it's so negligible that it doesn't worth the discussion.
I don't care which of them you take, as long as the issue is fixed 👍 😉

@ricardograca

Great. This just needs a test case then and I'll merge it.

@osher

This comment has been minimized.

Contributor

osher commented Feb 7, 2018

@osher

This comment has been minimized.

Contributor

osher commented Feb 8, 2018

ok, done. There you go 😃
@ricardograca - I think the ball is in your court again

Updated: Sorry it took so long, I misread the picture...

@osher

This comment has been minimized.

Contributor

osher commented Feb 8, 2018

@ricardograca
Uh...
Is there something I have to do to handle the change-request except the commits I pushed?

@ricardograca

This comment has been minimized.

Member

ricardograca commented Feb 8, 2018

@osher Sorry, I'll take a look later today. Currently busy with something else.

@ricardograca ricardograca moved this from To Do to In Progress in Version 0.13.0 Feb 8, 2018

@ricardograca ricardograca merged commit 9f0f243 into bookshelf:master Feb 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Version 0.13.0 automation moved this from In Progress to Done Feb 8, 2018

@osher osher deleted the osher:patch-2 branch Feb 11, 2018

@osher

This comment has been minimized.

Contributor

osher commented Feb 11, 2018

+1 👍

@ricardograca ricardograca added the bug label Feb 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment