Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support for add_on_usage #130

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ordo-viddler
Copy link

Here is my attempt to implement add_on_usage: https://dev.recurly.com/docs/usage-record-object.

Couple things I would like you to be sure to check:

  1. naming - english is not my native, so if you find anything should be named different, let me know
  2. I introduced NullFloat as it seems to be necessary in one of usage field
  3. I changed PagerOptions query field to be public (Query) - add on usage record utilizies some non-standard query parameters. If this should be done different way, please let me know and I'll fix that.

Thank you.

Copy link
Member

@cristiangraz cristiangraz left a comment

Choose a reason for hiding this comment

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

Thanks so much for contributing!

I will need to give this one more (thorough) pass as I am tight on time, but I wanted to leave some initial comments in the meantime on a few changes that stood out to me right away.

pager.go Show resolved Hide resolved
add_on_usage.go Outdated Show resolved Hide resolved
add_on_usage.go Outdated Show resolved Hide resolved
add_on_usage.go Outdated Show resolved Hide resolved
add_on_usage.go Outdated Show resolved Hide resolved
add_on_usage_test.go Outdated Show resolved Hide resolved
pager.go Outdated Show resolved Hide resolved
add_on_usage.go Outdated Show resolved Hide resolved
xml_test.go Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@ordo-viddler
Copy link
Author

Hi!

I answered your questions and fixed what you requested.

I'm terrible sorry that I did this 8 days after your request, but I was on vacation.

Thanks and let me know if any further information are needed.

Copy link
Author

@ordo-viddler ordo-viddler left a comment

Choose a reason for hiding this comment

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

Changes applied (Still query -> Query) to be resolved.

@ordo-viddler
Copy link
Author

Hi there!

Thank you for checking and reviewing my attempt for to add_on_usage support in you library.
As it appears it does not fit, I'll close this PR to keep things clean.

Thanks!

@cristiangraz
Copy link
Member

Hi @ordo-viddler, apologies for the delay as I've been very busy.

I really appreciate the PR -- the holdup has been mostly thinking about the query param being exported, and the time to look through the Recurly docs to gather more context. Would love to have you reopen this so we can add to the repo.

@ordo-viddler ordo-viddler reopened this Nov 4, 2019
@ordo-viddler
Copy link
Author

Hi Sure, I just wanted to keep things clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants