dkubb / axiom

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

Axiom types refactor #36

Closed dkubb closed 11 years ago

dkubb commented 11 years ago

This branch will include a refactoring of axiom to use axiom-types.

dkubb commented 11 years ago

@cored ok I think I have something working that passes the tests.

There's some performance issues with this though, the specs run much slower than before and I think it's in Attribute.infer_type, which I left a comment on.

I also think we should audit the Attribute subclasses and make sure our quick patches didn't miss anything. Ideally we want to remove as much as possible from each Attribute subclass, and delegate things to the type, when possible. I've already left comments hinting in this direction, but I'm sure there's more we can do.

I am going to do a pass over the diff and look at everything we did to make sure there's no extraneous stuff, and make sure it's optimal. Feel free to do the same if you want, the more of us looking at this the better.

dkubb commented 11 years ago

@cored I just added the --warnings flag to rspec and saw a ton of warnings in axiom-types. I will get those fixed up today, but I wanted to give you the heads up so you didn't think it was something you did.

dkubb commented 11 years ago

@cored on the TODO list, I have some notes about removing #range and delegating #minimum and #maximum to the type. I wonder if those are even necessary? Maybe we should just start by removing the #range method and not worry about delegating those other methods until a concrete need appears? wdyt?

dkubb commented 11 years ago

@cored I also noticed that we have things like #min_length in String, and it only seems to be used for equalizer's benefit. If we changed Axiom::Attribute itself to equalize on the name, type and required?, then we could eliminate these methods.

Previously all these methods were needed in Attribute because it was handling constraints and validations. Now all that is handled by axiom-types. Maybe someday we'll need a way to get to the type constraints through the attribute, but I would rather remove code until that is totally required.

dkubb commented 11 years ago

@cored after thinking about this for a while, I think it is probably unnecessary to completely eliminate the Axiom::Attribute subclasses. I think anything we do to create an intermediary structure to link together methods and types will end up being far more complex than Axiom::Attribute subclasses.

However, I do think we should continue with removing functionality that is duplicated between them. Please note this does not change anything we've done up to this point, and does not affect anything still to be done in this PR. All it means is that once the todo notes in this PR are complete and this code is merged into master, we're essentially done with the integration.