search

tapajos / highrise

Ruby wrapper around Highrise API.
MIT License
124 stars 77 forks source link

Changed tags to return ActiveResource objects rather than arrays #70

Closed basvodde closed 8 years ago

basvodde commented 10 years ago

This pull request is a bit harder than the previous ones (actually delayed this one for a while to think about the specs). The essence is simple. The tags method should return ActiveResource objects (in this case Highrise::Tag) rather than Hash. If it doesn't, then it causes different behavior between dealing with an existing Person or with a new Person. (It was actually quite hard to figure this one out when I got a failure thrown at me when running in production).

Earlier version used get(:tags) which returns an unprocessed Hash. It was hard (stackoverflow said impossible) to change that in a Highrise::Tag object. Now it uses the normal find interface to find 'itself' when the tags attribute do not exist yet and then it will copy the tags from that object into itself. This ensures the objects are of the right type.

Initially, I had only fixed this for Person and removed it out of the Taggable mixin. I was able to generate the implementation to also work for Company (using self.class instead of Highrise::Person) but the specs are more difficult to generalize, so I left them in Person for now.

The pull request is important as without it I can't run my code in production. However, would be open for discussion on solving this in a different way. It works but I'm not incredibly happy with it. (in fact, I'm not happy that Highrise does as if people and companies are the same thing! but that is a different story)

basvodde commented 10 years ago

Any update on this one?

lucasmartins commented 10 years ago

Sorry @basvodde, I have no more focus to maintain this gem. Not only that, I do not use it.

You seem to be using Highrise and willing to write code for the Gem, do you want to maintain it?

I suggest you keep your fork as the main fork for now, and talk to @tapajos about how to release the packages. We can update both the Readme and the GitHub gem description to link your fork.

What do you think?

basvodde commented 10 years ago

Uhm, I'm not sure that is a good idea. There are a couple reasons for that. First, I'm maintaining several other projects. Some of them are so active that it keeps me quite occupied and adding one wasn't what I had in mind. Second, I'm afraid that my highrise effort is not permanent. I mean, I'm not writing some code for that, but most of that is done and I'm just gradually putting some of the generic code back to this gem. When that is done, then my focus on highrise will probably decrease again. Third, so far most pointers for the highrise gem point to @tapajos and I'm not sure there is any benefit to changing that.

I'm willing to contribute to this repository still and help out with open issues and future pull requests, but probably not as the sole maintainer. And I do appreciate quick checks over my own pull requests as some of your previous feedback been useful.

basvodde commented 10 years ago

Any update on this? Could you merge this one or add me as committer so that I can merge it myself :P I'll do all changes via PRs still but then can leave them open for a week or so...

basvodde commented 10 years ago

Merge please :) (or add as committer)

lucasmartins commented 8 years ago

Sorry, I've got lost in my GitHub duties.

I don't have the permission to add committers, : (

tapajos commented 8 years ago

You don't have but I do!

@basvodde you are a committer. I would ask you to keep doing PR for all changes even if you will be merging it.

basvodde commented 8 years ago

Thanks, will do. It hasn't been my focus the last months, but I think I'll be back to it at some point. And I might check the existing PRs for fun.