raven.Client should support comparison

[Julian]

It's useful to compare two instances (via e.g. ==), especially when we write test cases and want to assert about how a logging handler has been configured.

[dcramer]

Can you describe the use case? An equal check here is fairly complex, and in every test case we've had we simply want identity checking (client is client).

[Julian]

I have some code that constructs a client, and I need to unit test it, essentially.

is is fine (well, a bit non ideal but fine-ish) if you've got a flat object, but another we've run into is -- what have you done when you have a compound object (an object with a client as an attribute) -- since raven.Client does not support ==, now your compound object can't easily either.

I think it shouldn't be too bad to do this, no? Despite whatever complexity happens in __init__, just store the parameters passed to it, and use those for comparison. Right now the DSN that was passed in is just thrown away. Happy to send a PR of course if that sounds reasonable and I haven't missed anything.

[ashwoods]

I really can't say why, but intuitively I'm against changing the default behavior without a major reason. raven.Client does support equality, class instances compare by object identity by default.

I'm not entirely sure what the general expectation is for equality in an object without an intrinsic value would be. Choosing the arguments for the constructor method is somewhat arbitrary, as class instances can be modified later. Comparing __dict__ might be more sensible, but a complete implementation would include __eq__,__ne__, and __hash__, would break code that for any reason might depend on the default functionality (tbh can't think of a sensible use case either, but who knows), and would run into the same problem for any object referenced in __dict__ if we implement a strict comparison with ==.

I'd say it's just easier to modify the tests to either assert identity, or subclass raven.Client for your tests if there is no other way. what do you think @dcramer ?

[Julian]

OK, sounds like there's possibly an idealogical difference here, so might just be worth closing this.

FWIW, attrs exists mostly to answer your second paragraph.

And I disagree with your last one, I think it's a test smell to ever assert about identity (identity is an implementation detail of some code, so unless the test is some test asserting about a performance optimization, it's not appropriate IMO), but probably that's tied into the idealogical difference above.

Happy to close though if this isn't likely to be acceptable, we've worked around this in somewhat obvious ways (similar to what you mentioned, although it's not that simple, you also need to write code to inject in the subclass everywhere that constructs one, and more likely what you end up doing is literally just adding this behavior to a subclass and using that everywhere instead of the real one, which is basically "do this, but not have it upstream" :).

[ashwoods]

You could monkeypatch __eq__ on test runner init and have it always return True :)
Never shy of ideological discussions, but really here to find solutions.

I'm a big fan of attrs, can't use it here as we are aiming to continue support for 2.6, 3.2 and 3.3, if possible. I think attrs helps the how (by reducing boilerplate, and in the neat concise readable declarative syntax), but in my opinion, not the what.

attrs default of using all attributes is ok for better than namedtuple like classes, but in more complex objects it often doesn't make sense, and here, I honestly don't know. Before opening a possible can of worms I'd like to have a clear picture of the implications and that the choice is not arbitrary if it doesn't have to.

I'm prototyping several breaking changes anyway, so I'd leave this open and set it for consideration for 7.0.

[Julian]

You could monkeypatch eq on test runner init and have it always return True :)

I don't want it to return True, I want the "proper" result, which yeah I can monkeypatch -- I wanted to file a ticket to see if that monkeypatching could be pushed upstream, but if that's my only option yeah, we've already done that essentially.

attrs default of using all attributes is ok for better than namedtuple like classes, but in more complex objects it often doesn't make sense,

I'd say "don't have complex objects" :)

But happy to see what you have in store for 7.0, and appreciate the time.