search

DoubleYouGTT / hubspot

R package 📦 for working with Hubspot 👩‍💼👨‍💼 data
https://itsalocke.com/hubspot/
Other
10 stars 8 forks source link

modified get_deals to use property_history flag #64

Closed seanofahey closed 4 years ago

seanofahey commented 4 years ago

This fix addresses issue #63 using the first of the two potential fix approaches

maelle commented 4 years ago

Thanks @seanofahey!

I would like to add a test for the behaviour but at the moment, with the "demo" key I don't see any difference when I set property_history FALSE or TRUE.

@stephlocke I think we should make Boolean parameters logical (e.g. FALSE not "false"), but that's a breaking change.

maelle commented 4 years ago

I've just found an example, I'll add a test.

stephlocke commented 4 years ago

I made the values "true" and "false" because it:

  1. reduces the code requirement to turn the booleans consistently into the format expected by hubspot's API which is case sensitive
  2. means that if they add a third value like "latest", the working with strings means no refactoring to support it

As much as TRUE/FALSE is the more R way of doing things normally, I'd prefer to keep things as strings for now.

Thanks for the PR(s) by the way @seanofahey!

maelle commented 4 years ago

Ok, I'll revert that and then merge, thanks @stephlocke . Thanks again @seanofahey!

seanofahey commented 4 years ago

Happy to help. Thanks for the great project.

On Jan 6, 2020, at 8:24 AM, Maëlle Salmon notifications@github.com wrote:

 Ok, I'll revert that and then merge, thanks @stephlocke . Thanks again @seanofahey!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.