search

estately / rets

A pure-ruby library for fetching data from RETS servers
https://github.com/estately/rets
127 stars 94 forks source link

Big metadata refactoring (move logic out of initializers and remove reliance on mutation) #145

Closed hfaulds closed 9 years ago

hfaulds commented 9 years ago

This is a pretty humongous PR. Honestly I'm unsure about the logistics of whether it is preferable to split this up or not. I tried to keep the individual commits pretty reviewable.

The main focus of this PR was to remove all the logic from the various metadata classes' initializers.

The print_tree test is a great example of how much easier this makes testing. One can setup a tree just using initializers, whereas previously several classes were constructed and then mutated (RetsClass and Resource in particular). I also tried as best I could to reduce the mocks and stubbing in the tests. This makes them a whole lot more readible imo and less prone to breaking.

I didn't touch Metadata::Root in this PR but other than that the tree structure is no longer self-referential and should be persistable rather than having to persist the raw xml as we do at the moment. Hopefully this will mean we can start to filter out parts of the metadata tree we aren't using (i.e. only persist the Resources/Classes we actually need).

This should make testing a case insensitive resolve option far more testible as well.

tcrayford commented 9 years ago

looked over this, and I think it's good and ready to merge.

tcrayford commented 9 years ago

(assuming it's tested ofc)

hfaulds commented 9 years ago

I can't see this breaking any caching because we cache the raw metadata atm but it would be easier to verify once we have https://github.com/estately/rets/pull/134 in

I'm going to putt his on hold until we make a decision on that PR

phiggins commented 9 years ago

This is a bit much to read and comprehend in one go, but seems pretty positive to me. :+1:

tcrayford commented 9 years ago

@phiggins I found it was ok if you just read commit by commit. Otherwise there's too many changes to review sensibly. Commit by commit it makes a lot of sense to me.

hfaulds commented 9 years ago

Testing this in production

hfaulds commented 9 years ago

Fixed the one issue that we encountered in production (tables not having long_name accessible).

This is good to go imo.