rails / arel

A Relational Algebra
2.06k stars 390 forks source link

Add missing hash, eql?, == to various node classes #490

Closed MaxLap closed 7 years ago

MaxLap commented 7 years ago

Some of the nodes classes are missing either one or many of the common comparison methods #hash, #eql? and #==.

This makes comparision and working with the ast sometimes painful, as equality or operations likes array differences (which uses a hash behind the scene) produces unexpected results.

A test has been added that ensures that every descendants of Node:

rails-bot commented 7 years ago

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

sgrif commented 7 years ago

These objects aren't generally meant to be used as hash keys. Can you give an example of where this is actually causing an issue in Rails or another consumer of Arel?

MaxLap commented 7 years ago

Well, most of the nodes already define those methods, so i'm assuming there is already some kind of use case.

If you have an array of nodes, and you want to use array methods like #delete, you need equality to work correctly. If you want to use #- or #uniq, then you need to have the hash and eql? since that's what ruby uses behind the scene. So this isn't just about being used as hash key directly.

An explicit use case would be to be able to remove duplicate nodes from an array of nodes.

sgrif commented 7 years ago

I suppose there's enough places already defining these to justify it.