Skip to content

Conversation

@yoshicoverhound
Copy link

For all values other than email and phone, the required format for the keys is very strange. I thought that this could be one way to improve this, or at least put more of this weirdness under the hood.

Happy to update or get shot down if this isn't going to work, just an idea.

For all values other than email and phone, the required format for the keys is very strange. I thought that this could be one way to improve this, or at least put more of this weirdness under the hood.

Happy to update or get shot down if this isn't going to work, just an idea.
Copy link
Contributor

@berfarah berfarah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoshicoverhound Good thinking. I have a few suggestions:

  1. Not all resources require these attributes. By putting this logic in the base-class it'll be tricky to work with anything that is not the Contact. Is there a reason you wanted it in Resource?
  2. It seems like parameter formatting is something that may evolve and have more complexity down the line. Do you mind extracting that into a class that handles formatting and validation?
  3. Gems will often have their own error type that wraps all internal errors so that the consumer can safely handle them. It might make sense to add a base ActiveCampaign error that inherits from StandardError.
  4. It looks like raising parameter errors could be a common use-case in the future. Do you think it makes sense to create a custom error class for this?

@yoshicoverhound
Copy link
Author

thanks for the feedback @berfarah

  1. I thought that Contact really was the only resource, but maybe I'm mistaken? What other resources are there?
    2-4. All make sense to do, just didn't want to start extracting and changing without making sure the initial idea is sound.

@berfarah
Copy link
Contributor

@yoshicoverhound Totes! You're right! Currently it's the only resource. That said, if we want to add more resources, it'd be good for them to be more general and have contact-specific stuff go there instead IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants