search

estately / rets

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

Adjust object metadata #179

Closed summera closed 8 years ago

summera commented 8 years ago

This PR adds some adjustments to object metadata parsing and display.

First thing I noticed is that some rets servers use MimeType as the key for the object mime type. The gem was not picking this up in the metadata because it assumes the key to be MIMEType.

Also, the object type seems to be the differentiator between objects while the visible name is often equal to object type, sometimes just a generic name like Photo, or sometimes blank. So, I have made the object type the title in the printed metadata.

It may be useful to downcase hash keys when grabbing values for other fragments (classes, tables, etc.) but I didn't want to go that far unless it was necessary.

dougcole commented 8 years ago

I like it, thank you!

I understand not wanting to change too much, but I'd lean towards consistency between the different types of metadata classes, rather than having this one off change in the Object class. I think you could change all of them just by updating the compact parser (lib/rets/parser/compact.rb:70-85), but I haven't tested that and am not as familiar with the code as I used to be.

We've run into similar problems in the past, so fixing for most/all cases would be awesome.

dougcole commented 8 years ago

...that said if you don't have time to make the bigger change, just let me know. This improves the experience and can be improved on later.

summera commented 8 years ago

@dougcole am I correct in thinking that what you are suggesting in the compact parser (downcasing keys) would effect the keys for all resources and metadata throughout the gem and that this would break backwards compatibility for users who are parsing values from the metadata tree or property data?

dougcole commented 8 years ago

@summera: yeah, that's what I was thinking, although I have to admit I haven't done a ton of research so I could be wrong about either the implementation or the breaking of compatibility. Sorry for not being able to offer more support right now. I think the above changes are good to go if you want me to merge, I was just nudging you towards something bigger and maybe better :smile:

dougcole commented 8 years ago

merging as is - further improvements can be done later. Thanks @summera!

summera commented 8 years ago

@dougcole sounds good. Sorry I haven't gotten back to this! I still plan to add this improvement to the other parts of parsing once I have a chance to do so

dougcole commented 8 years ago

No worries - just figured I'd get this merged before it fell too far behind and had conflicts. Thanks for the contribution!