Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
6 changed files
with
62 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 This looks super useful!
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 agree
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As someone who has literally had to write "Order.select("id").collect(&:id)" four times today, this is a big +1 from me!
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've wanted a method like this for so long! I'm always writing little bits of code to do this. Thanks!
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 !
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @kennon, no SomeThing.select('col').map(&:col) any more!
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 Very useful!
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't try this out at the the moment, so here's a question: How would this compare to https://github.com/ernie/valium ? Same thing / functionality?
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pluck yeah ;)
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 really useful piece of code
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 ;)
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not several columns? Wouldn't it be as useful? or am I missing something here?
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+2 because it's that good
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it work with serialization like ernie/valium?
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmitriy-kiriyenko Since no one answered my previous question about how it compares to valium, and judging by the other comments, I'd say no one here actually knew about valium.
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cvshepherd just got to know valium, and I think it's better.
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is "pluck" really the right name for this ?
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1: Is "pluck" really the right name for this ?
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seriously, I would propose 'project' as a better name
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'project' is just as un-obvious. For a method that returns an array of columns I'd expect the method name to have something to do with getting an array of columns....
Something like 'selective_columns' and extend it to return optionally more than one column.
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not un-obvious if you've heard of an SQL projection
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
values_at
? =)a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I definitely like
project
. +1 forproject
.We'll have another reserved word to avoid in business code.
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, but I doubt it'll be changed anyway.
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being able to "pluck" multiple columns would be quite useful as well.
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 although I think values_at (what valium uses) would be a better name than pluck.
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, but I actually like "pluck". It's used pretty rarely in everyday speech, and to me seeing it is odd enough that I'll remember it as a method name. -- like "tap", the other most awesome method name ever! Plus, "pluck" is such a fun word!
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 , I actually think
values_at
is better thanpluck
orproject
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 multiple columns
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 multiple columns
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Green??? What a stupid color for a bikeshed.
But seriously, project is a better name ;)
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whitethunder, I told them, but they don't trust me. Seriously, project is an excellent name. More, I'm looking forward for a method like "user" or "company". Also a great idea would be methods "topic", "post" and "comment". =)
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For those who were asking, it looks like this patch handles serialized columns, because Column#type_cast decodes encoded columns in current master.
This wasn't the case in 3-0-stable, which is why Valium's implementation is (only slightly) more involved.
I agree with @jeremy, though -- this is a whole lot of discussion for a very simple change. In fact, I'd have submitted Valium's implementation as a patch long ago if I'd thought it had a chance to be accepted. One of those things where it was so ridiculously simple that I figured there was a reason it wasn't part of the AR API already. ;)
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I take back my comment about working with serialization. It looks like the only place that a Column's coder is being set in the current AR code is in 3 tests in column_definition_test at this point, unless I missed something. It doesn't look like SchemaCache would be the right place to handle this, either.
Anyway, I have a rough version of Valium's take on this ported to a Rails 3.2 patch and passing all but the serialization tests (due to the issue mentioned). I can work out the remaining issues there and submit a value(s)_of implementation for Rails 3.2 if the core team is interested.
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ernie Cool, yeah, let's see it. Wish we'd known you had Valium already implemented, sorry about that. Pull request came in; didn't look for prior art. Thanks for pitching in in any case.
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremy pushing it up now -- thanks!
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For those visiting this thread: See #3871 for the pull request with alternate implementation supporting serialization, multiple values, etc.
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tenderlove unicorns, rainbows and ponies would be the "etc" part
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it! +1
a382d60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, amazing +1