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

Rails 4.0 - bring back in-place editing support for collection associations #12236

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@georgiev

georgiev commented Sep 15, 2013

This PR is related to
#12140, #12213, 1a40be0 and #12227
Since personally I'm very unhappy with the way the resolution of these issues is heading, and I thing it's killing the fun in Rails and forces me to write awkward code - here goes my offer

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 15, 2013

Member

I think I'm missing something. Are not the in-place edit, methods still deprecated in your PR?

Member

rafaelfranca commented Sep 15, 2013

I think I'm missing something. Are not the in-place edit, methods still deprecated in your PR?

@georgiev

This comment has been minimized.

Show comment
Hide comment
@georgiev

georgiev Sep 15, 2013

Not for the CollectionProxy - it overrides array_delegate().
I left the deprecation in the default implementation as I'm not aware of all usage scenarios.

georgiev commented Sep 15, 2013

Not for the CollectionProxy - it overrides array_delegate().
I left the deprecation in the default implementation as I'm not aware of all usage scenarios.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 16, 2013

Member

I think you missunderstood. This behavior was changed by design. We choose to not support this in Rails 4.0 and we will not bring it back.

Member

rafaelfranca commented Sep 16, 2013

I think you missunderstood. This behavior was changed by design. We choose to not support this in Rails 4.0 and we will not bring it back.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 16, 2013

Member

Thank you for your pull request.

Member

rafaelfranca commented Sep 16, 2013

Thank you for your pull request.

@georgiev

This comment has been minimized.

Show comment
Hide comment
@georgiev

georgiev Sep 16, 2013

Can I see the discussion somewhere ?

georgiev commented Sep 16, 2013

Can I see the discussion somewhere ?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 16, 2013

Member

Unfortunately no. It was in our private campfire room

Member

rafaelfranca commented Sep 16, 2013

Unfortunately no. It was in our private campfire room

@georgiev

This comment has been minimized.

Show comment
Hide comment
@georgiev

georgiev Sep 16, 2013

Can you tell In two words - why did you chose to do that?

georgiev commented Sep 16, 2013

Can you tell In two words - why did you chose to do that?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 16, 2013

Member

But this change is on the CHANGELOG entry 0a6833b

Member

rafaelfranca commented Sep 16, 2013

But this change is on the CHANGELOG entry 0a6833b

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 16, 2013

Member

Because it causes confusion where you think you are changing the relation/association and you actually is not. The database is still the same.

Member

rafaelfranca commented Sep 16, 2013

Because it causes confusion where you think you are changing the relation/association and you actually is not. The database is still the same.

@georgiev

This comment has been minimized.

Show comment
Hide comment
@georgiev

georgiev Sep 16, 2013

Sorry but all that I can see is more awkward code instead has_many.select!{ my records } I'll have to do has_many = has_many.select{ my records } , the results is the same only more difficult to achieve.

georgiev commented Sep 16, 2013

Sorry but all that I can see is more awkward code instead has_many.select!{ my records } I'll have to do has_many = has_many.select{ my records } , the results is the same only more difficult to achieve.

@georgiev

This comment has been minimized.

Show comment
Hide comment
@georgiev

georgiev Sep 16, 2013

and with more array dupping

georgiev commented Sep 16, 2013

and with more array dupping

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 16, 2013

Member

Yup, this is another thing we want to avoid. This kind of operation is not performatic if you have a huge set of data. If you want to do select you should be using the query interface, not the array methods.

Member

rafaelfranca commented Sep 16, 2013

Yup, this is another thing we want to avoid. This kind of operation is not performatic if you have a huge set of data. If you want to do select you should be using the query interface, not the array methods.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 16, 2013

Member

But if you want to do in-place editing you still can:

has_many_array = has_many.to_a

has_many_array.select! { }
Member

rafaelfranca commented Sep 16, 2013

But if you want to do in-place editing you still can:

has_many_array = has_many.to_a

has_many_array.select! { }
@georgiev

This comment has been minimized.

Show comment
Hide comment
@georgiev

georgiev Sep 16, 2013

Yea I know that but how can I tell back to the association that it should unlink some records when the owner is saved?, This hurts autosave cascading.

georgiev commented Sep 16, 2013

Yea I know that but how can I tell back to the association that it should unlink some records when the owner is saved?, This hurts autosave cascading.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca
Member

rafaelfranca commented Sep 16, 2013

Using replace?

@georgiev

This comment has been minimized.

Show comment
Hide comment
@georgiev

georgiev Sep 16, 2013

As far as I see this is instant and will not bundle with owner save transaction. Am I wrong?

georgiev commented Sep 16, 2013

As far as I see this is instant and will not bundle with owner save transaction. Am I wrong?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 16, 2013

Member

You can also set the association. page.comments = new_comments

Member

rafaelfranca commented Sep 16, 2013

You can also set the association. page.comments = new_comments

@georgiev

This comment has been minimized.

Show comment
Hide comment
@georgiev

georgiev Sep 16, 2013

Which brings us exactly where we started - I'll have to use assignment to throw out some comments.

georgiev commented Sep 16, 2013

Which brings us exactly where we started - I'll have to use assignment to throw out some comments.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 16, 2013

Member

Yes. And it is fine. I don't see any problem on using it. The decision was already made and like I said we will not change it back since it introduce more confusion than feature.

We prefer to make the things explicit.

Thank you for the discussion.

Member

rafaelfranca commented Sep 16, 2013

Yes. And it is fine. I don't see any problem on using it. The decision was already made and like I said we will not change it back since it introduce more confusion than feature.

We prefer to make the things explicit.

Thank you for the discussion.

@georgiev

This comment has been minimized.

Show comment
Hide comment
@georgiev

georgiev Sep 16, 2013

Ok - beer for all of you people!

Just a few final questions:
In the CHANGELOG I see WHAT is done but nothing about WHY?
Why should I write more code to achieve the same thing?
In application level terms - what is belongs_to - the object I belong to right?
What is has_one - the object that I have right?
What is has_many - a COLLECTION of objects that I have right?
If the above is right why is the has_many NOT acting as a collection (I don't really care if its Relation or Array)?
When is has_many acting as a collection and when it's not, which methods are available and which are not?
Should I always call to_a just to be sure that all collection methods are available?
If the above is true why don't you call it for me?
And how is duplicating the array for each and every array delegated method efficient by any means?
How has_many.build aligns with your concept of no in-place modifications?
If you want to make things explicit - why don't you drop the whole array delegation (it's inefficient anyway)? That's what will make things explicit.

Anyway it was really nice discussion.
Thank you too.

georgiev commented Sep 16, 2013

Ok - beer for all of you people!

Just a few final questions:
In the CHANGELOG I see WHAT is done but nothing about WHY?
Why should I write more code to achieve the same thing?
In application level terms - what is belongs_to - the object I belong to right?
What is has_one - the object that I have right?
What is has_many - a COLLECTION of objects that I have right?
If the above is right why is the has_many NOT acting as a collection (I don't really care if its Relation or Array)?
When is has_many acting as a collection and when it's not, which methods are available and which are not?
Should I always call to_a just to be sure that all collection methods are available?
If the above is true why don't you call it for me?
And how is duplicating the array for each and every array delegated method efficient by any means?
How has_many.build aligns with your concept of no in-place modifications?
If you want to make things explicit - why don't you drop the whole array delegation (it's inefficient anyway)? That's what will make things explicit.

Anyway it was really nice discussion.
Thank you too.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 20, 2013

Member

@georgiev These are good point. I'll ask the Core teams opinions on this subject.

Member

rafaelfranca commented Sep 20, 2013

@georgiev These are good point. I'll ask the Core teams opinions on this subject.

@joshjordan

This comment has been minimized.

Show comment
Hide comment
@joshjordan

joshjordan Sep 24, 2013

Contributor

@rafaelfranca "Because it causes confusion where you think you are changing the relation/association and you actually is not. The database is still the same."

^ Really? Who would think that? That's ridiculous.

You seem to be designing for people that have very little understanding of the system that they're working on, rather than the common case. Further, a single statement of warning in documentation should solve this problem. Someone would need to try it only once to understand. This direction seems so inconsistent with the entire concept of editing ActiveRecord objects in memory.

Contributor

joshjordan commented Sep 24, 2013

@rafaelfranca "Because it causes confusion where you think you are changing the relation/association and you actually is not. The database is still the same."

^ Really? Who would think that? That's ridiculous.

You seem to be designing for people that have very little understanding of the system that they're working on, rather than the common case. Further, a single statement of warning in documentation should solve this problem. Someone would need to try it only once to understand. This direction seems so inconsistent with the entire concept of editing ActiveRecord objects in memory.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 24, 2013

Member

ridiculous

@joshjordan 💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙

Member

rafaelfranca commented Sep 24, 2013

ridiculous

@joshjordan 💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙💚💛❤️💜💙

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