dkubb / axiom

Simplifies querying of structured data using relational algebra
https://github.com/dkubb/axiom
MIT License
457 stars 22 forks source link

Attribute should not freeze its primitive type #46

Closed solnic closed 10 years ago

solnic commented 11 years ago

This is too aggressive, axiom should not freeze globally available class objects. This can break people's code and currently it breaks all specs in rom-session because rspec tries to monkey-patch BasicObject which happens to be frozen.

mbj commented 11 years ago

@dkubb. Lets default ice_nine to skip instances of Class. I think it creates more problems. Only our community does like this kind of freezing, and I think we can freeze our, sometimes dynamically, created classes by ourselves.

If we still recurse into ivars of Class should be discussed.

solnic commented 11 years ago

Yes I think we should be extra careful when using Adamantium with deep freezing. This can and probably will piss off many people if our libs freeze things they don't own.

mbj commented 11 years ago

We could also provide a mixin Adamantium::ClassFreeze that "whitelist our classes" to be frozen. Or whitelist the namespaces we "own".

solnic commented 11 years ago

I don't think it makes sense. Open classes is one of the ruby features. We could not agree with it but it doesn't matter. A ton of ruby libs change classes through monkey-patching whether we like it or not and we should be good citizens here and skip freezing classes just because of that.

It's totally fine to freeze stuff we generate, as you said. So once we start generating mappers on the fly we could freeze them.

mbj commented 11 years ago

@solnic I'm okay with freezing some of my classes. But I dont think I'll invest time into this soon.

snusnu commented 11 years ago

@solnic @mbj @dkubb imo our support code MUST allow us to not freeze anything we do not own. It's ok that we (deep) freeze stuff when we own the complete object graph, but that freezing should never step outside our own boundaries without us having a chance to stop it from doing so. It's simply not friendly to alter code we do not own, and it can be the source of subtle, annoying bugs.

snusnu commented 11 years ago

There's also a case where equalizer deep freezes stuff in the object graph, and there's no way of stopping that. I see no reason why equality behavior must be tied to deep frozen instances only.

For reference, this code demonstrates that problem with equalizer.

snusnu commented 11 years ago

In fact, I think we're doing exactly what we don't want others to do. Modifying ruby's behavior globally. We typically don't like core extensions too much, because they change the behavior of already existing classes. If we start to (deep) freeze people's code, we're essentially doing the same. Modify it. If only for the sake of not being able to modify it anymore.

solnic commented 11 years ago

@snusnu current "version" of equalizer used in axiom automatically includes Adamantium when you include an equalizer module

We should update axiom to use the extracted version of equalizer and include adamantium separately using "flat" module where appropriate

snusnu commented 11 years ago

@solnic agreed

solnic commented 11 years ago

@dkubb bump...

dkubb commented 10 years ago

This should no longer be an issue because Attribute no longer holds a reference to a primitive, but rather an Axiom::Types::Type instance.