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

Added a case converter plugin #1093

Merged
merged 2 commits into from
Jan 12, 2018
Merged

Added a case converter plugin #1093

merged 2 commits into from
Jan 12, 2018

Conversation

kripod
Copy link
Contributor

@kripod kripod commented Jan 15, 2016

The case converter plugin handles the conversion between the DB's snake_cased and JS's camelCased Model properties automatically.

It is not hard to implement manually by anyone, but as many people use similar code, I think that this functionality deserves its own official plugin.

@ricardograca
Copy link
Member

There's been some talk about moving this functionality into core Bookshelf (not a plugin and not using parse and format) for some time. You can check out the conversations about that if you search for "format is confusing" and issues dealing with both methods.

I'm not saying this won't get accepted, but it's a bit against the general direction for the future, so let's see what the other guys think.

@kripod
Copy link
Contributor Author

kripod commented Jan 15, 2016

Thanks for the reply! Then, there could be a boolean option for initializing Bookshelf instances, indicating whether automatic case conversion should happen.

Merge with remote branch
@vellotis
Copy link
Contributor

@ricardograca Just to philosophically continue the converisation. What will be the future as this project is not actively developed but just maintained?

@ricardograca
Copy link
Member

ricardograca commented May 22, 2016

That question is in no way related to this issue, so please use the appropriate communication channels for that, like Gitter or a relevant GitHub issue.

@rhys-vdw
Copy link
Contributor

@kripod Yeah, this is a great idea. I'd like simple test cases for this just to be sure. Also this should be documented as the default way to achieve this, leaving manual parse/format overrides as more "advanced" approaches.

@ricardograca I don't think that the idea of key conversion for records will go away, we'll just have to update/remove the plugin as the feature evolves.

Copy link
Contributor

@chamini2 chamini2 left a comment

Choose a reason for hiding this comment

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

Is there any way to make the conversion functions parameters?

return _.reduce(attrs, function (memo, val, key) {
memo[_.camelCase(key)] = val;
return memo;
}, {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not do instead:

parse: function(attrs) {
  return _.mapKeys(attrs, (, k) => _.camelCase(k));
}

Copy link
Member

Choose a reason for hiding this comment

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

I'll convert to that method after merging this, since the original author isn't interested in doing it.

@ricardograca
Copy link
Member

@kripod If you can address the comment by the reviewer and provide some tests we can merge this.

@kripod
Copy link
Contributor Author

kripod commented Jan 7, 2018

@ricardograce I'm sorry, but unfortunately, I lost my interest for this project.

@ricardograca
Copy link
Member

Merging this as is and then I'll add more documentation to it and change the method used as proposed by @chamini2. The difference in performance is negligible between the two, but it uses less code, and is not less readable, so there's that.

Copy link
Member

@ricardograca ricardograca left a comment

Choose a reason for hiding this comment

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

This is a good enough starting point.

@ricardograca ricardograca merged commit 7de5660 into bookshelf:master Jan 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants