Return the result of calling the super implementation.

[lowjoel]

No description provided.

[coveralls]

Coverage remained the same at 99.44% when pulling 1679d06 on lowjoel:master into bc3ebf6 on SchemaPlus:master.

[ronen]

@lowjoel Yeah that's the proper form for return the results... but I didn't realize that has_many etc. return a result of interest. I don't see it documented anywhere...

... and looking at the 4.2.1 source they just call Reflection.add_reflection, which merges the new reflection into the ar._reflections hash--and since that's the last line of code that's what gets returned. That seems like a side-effect rather than intended return value.

So what's the use case for wanting this? If you want the reflections, wouldn't it be safer to just call model.reflections after model.has_many?

[lowjoel]

I didn't know either -.- I won't (and don't) write my code in this manner.

...but activerecord-acts_as depends on the return result of that. I think we have to maintain compatibility as far as possible. By right, if we are patching methods, we should always return the super call's result, regardless of whether we intended it or not.

[lowjoel]

See hzamani/active_record-acts_as/master/lib/active_record/acts_as/relation.rb#L52

[ronen]

I think we have to maintain compatibility as far as possible.

OK Fair enough. (Though if some other gem relies on undocumented internal behavior of AR, at some point our meddling is likely to conflict with theirs.)

I'll add a comment saying saying that rails doesn't document the return value, but some gems apparently rely on it anyway.

By right, if we are patching methods, we should always return the super call's result, regardless of whether we intended it or not.

Yeah you're probably right. (On the other hand, if anybody relies on undocumented internal features of rails, they have to expect their code to break at some point. And I'd hate to accidentally set up schema_plus to start having to support a particular arbitrary return value even if AR later changes. And it could be confusing for future maintainers as to why we're going to lengths to return a nonsense value. And it could confuse users to see a result field in an env for a method that has no useful result value.)

Anyway for now I'll stick to just merging your changes ,and add the comments. But not add documentation about the return value.

[ronen]

...released 0.3.1

[lowjoel]

thanks @ronen!

I don't support using undocumented return values, but the nature of Ruby is such that it's not always obvious what the return result of a method is. Therefore, as a matter of principle, whenever I'm doing metaprogramming I always pass the original implementation's result back. I do not process the result.

If someone depends on the undocumented return result of some Rails API, then if it breaks in some later version, it's his problem. The job of a middleware should be to faithfully return whatever the underlying implementation does if no filters are installed (a constraint that I unintentionally broke). That's kind of why I thought it counterintuitive that I had to explicitly capture the return value and return it from the middleware stack.

Not sure how these ideas can be reconciled. Thoughts?

[ronen]

I don't support using undocumented return values, but the nature of Ruby is such that it's not always obvious what the return result of a method is. Therefore, as a matter of principle, whenever I'm doing metaprogramming I always pass the original implementation's result back. I do not process the result.

Yes, that's a reasonable policy. You're arguably right, schema_plus_core should arguably as a matter of course pass back all return values from methods it overrides. (It just rankles me to deliberately put in code that is intended to be pointless, can't easily be tested, and so forth. Really I'd be just as happy to file a PR to actve_record-acts-as that gets rid of their dependency on the undocumented return value.)

If someone depends on the undocumented return result of some Rails API, then if it breaks in some later version, it's his problem.

Agreed.

The job of a middleware should be to faithfully return whatever the underlying implementation does if no filters are installed (a constraint that I unintentionally broke). That's kind of why I thought it counterintuitive that I had to explicitly capture the return value and return it from the middleware stack.

Not sure how these ideas can be reconciled. Thoughts?

Here's my thoughts...

I'd say that, following your initial point, it's schema_plus_core's job to faithfully return the same value as any method that it overrides.

But modware isn't the same as schema_plus_core. modware's job is to implement the "middleware stack pattern". And the "middleware stack pattern" by its nature is intended to let you modify the results of earlier calls in the stack. The common mechanism to do that is to pass around some sort of shared environment object and require that each middleware in the stack does its work by reading from and writing to that environment object. E.g., the canonical/definitive example in ruby, Rack middleware, passes the request/response object as the shared environment, and AFAIK doesn't have any notion or convention of returning a value from @app.call from a lower level in the stack.

So if schema_plus_core wants to use a middleware stack internally in order to filter the results of a builtin AR method, it's up to schema_plus_core to provide the linkage between the function call paradigm and the middleware stack paradigm -- which it does by creating a middleware method which simply takes parameters from the env, calls the AR method, and stores its return value in the env.

From that perspective, I don't think it's particularly counterintuitive -- schema_plus_core completely wraps the the original method: both its parameters and its return value go through the env.

Perhaps schema_plus_core could have some wrapper or metaprogramming to simplify that wrapping task, which is admittedly mostly boilerplate. But I can't think of anything that seems particularly cleaner than the boilerplate code itself.

[lowjoel]

Yes, that's a reasonable policy. You're arguably right, schema_plus_core should arguably as a matter of course pass back all return values from methods it overrides. (It just rankles me to deliberately put in code that is intended to be pointless, can't easily be tested, and so forth.

Yes... I know that feeling :(

Really I'd be just as happy to file a PR to actve_record-acts-as that gets rid of their dependency on the undocumented return value.)

When I've got a bunch of stuff, I probably would. As a matter of fact I intend to actually come up with a cleaner solution than what they've got, supporting multi-level inheritance and things like that. Thing is, my experience was that PRs there weren't merged readily...

But modware isn't the same as schema_plus_core. modware's job is to implement the "middleware stack pattern". And the "middleware stack pattern" by its nature is intended to let you modify the results of earlier calls in the stack. The common mechanism to do that is to pass around some sort of shared environment object and require that each middleware in the stack does its work by reading from and writing to that environment object

I get you. That seems reasonable. It's probably my inexperience in general that would be to blame :)

Thanks for reviewing and merging this.

[ronen]

I get you. That seems reasonable. It's probably my inexperience in general that would be to blame :)

My seeming wisdom belies the number of crappy bad implementations i went through before i finally understood myself what i was trying to do :)