when calling the bang methods from QueryMethods on a model class are really not helpful.
It’s understandable that you don’t want to pass a model class into a method that expects a relation that it can act upon and change with a bang query method and have it successfully create a new relation and call some method on it that is never again seen, hence the error. I also understand wanting to avoid any more checking/raising code than necessary in ActiveRecord both for maintainability, clarity, and maybe efficiency- but just look at this:
I’m too late to the party, I thought these bang methods was public on rails 4
We will have people doing wrong things with these methods but a good documentation will reduce that, ruby itself have a lot of these methods like params.merge!(…) or controller_name.gsub!(…) and I never saw people (that do not means there is no people reporting, I just don’t saw) reporting issues because things like that.
The problem of scopes that must not be mutated can be solved using some freeze implementation on that scope, like this:
relation = User.where(:dumb => false).immutable!
relation.where!(:dumb => true)
#=> raises ActiveRecord::ImmutableRelation
I agree that new developers may do strange things but the rails itself is not so easy for beginners, but I have no expertise about how it may impact the community, so if you guys think that won’t be great these methods be public, :okaymeme:
The primary reason I would be against these being public is the following example:
User.where!(email: ‘bob@example.com’)
Reading that, I would expect that I have changed the default scope for EVERY user query. I believe this is what José Valim was hinting at. Because the ActiveRecord class is a global object, scopes originate from there, and any piece of code could modify the behavior of that global, it would introduce side effects throughout an application. Ideally, when side effects are present, they’re localized to a small scope.
That’s why the proposal of immutable relations, User.where! would raise a ActiveRecord::ImmutableRelation but must be a way to create mutable relations, Model.all would be fine since it returns new relations on each call (not sure about internals but it seems not too difficulty to make it work):
It is easier to teach people when we say “Relations are immutables” than “Relations are immutables, unless you call these methods, or someone called they in some plugin”.
Documentation will help, but it will not solve. In the same way you didn’t read that these methods are private API (they are not even listed in the API site) people will not read the gotcha in the documentation.
Also I don’t why we should complicate the relation API even more adding a new exception and new methods only to make possible to people able to call these bang methods.
It’s doing more than I’d like (having to create another array to wrap the result, and all of the additional operations in joins(…) vs. joins!(…)), but that’s ok.
We plan to move the API to YARD, hopefully for 4.1 unless there is something unexpected in the transition.
These would be use-cases for the @private tag I believe. The tag will convey user-level private visibility better than nodoc for those reading the source code.
