Skip to content

Conversation

rhuberdeau
Copy link

Fixes #555
Fixes #423

Prevents a stack level too deep error when to_json is called on a response object.

I did not add tests because the issue cannot be replicated at the gem level. However if you would like a test to prove it doesn't break anything I would be happy to include it.

There was no other documentation in the Domain class and since to_json and as_json are fairly well known I didn't think it was necessary to add any.

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • [x ] I have made a material change to the repo (functionality, testing, spelling, grammar)
  • [x ] I have read the Contribution Guidelines and my PR follows them
  • [x ] I have titled the PR appropriately
  • [x ] I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

@thinkingserious thinkingserious changed the title Fix: Adds to_json to response objects that inherit from Domain. fix: Adds to_json to response objects that inherit from Domain. May 14, 2021
@thinkingserious thinkingserious changed the title fix: Adds to_json to response objects that inherit from Domain. fix: Adds as_json to response objects that inherit from Domain. May 14, 2021
Copy link
Contributor

@thinkingserious thinkingserious left a comment

Choose a reason for hiding this comment

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

Could you please describe how you tested this? Thank you!

@@ -31,6 +31,10 @@ def request(method, uri, params = {}, data = {}, headers = {}, auth = nil, timeo
timeout
)
end

def as_json(*)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be to_json or as_json?

Copy link
Author

Choose a reason for hiding this comment

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

It should be as_json.

I tested this by updating the gem, locally, and then point my app to the local install. At that point my application did not get an error when calling to_json on an insights object.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a suitable unit test would be to ensure that the data returned by this function is as expected, located here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @rhuberdeau,

Just checking back in. Would you like us to take it from here or is this still WIP? Thanks!

@thinkingserious thinkingserious added status: waiting for feedback waiting for feedback from the submitter type: bug bug in the library labels May 14, 2021
@rhuberdeau
Copy link
Author

rhuberdeau commented Jun 16, 2021 via email

@thinkingserious
Copy link
Contributor

@rhuberdeau,

Do you mind pushing up what you have? Perhaps we can help. Thanks!

@rhuberdeau
Copy link
Author

@thinkingserious I merged the most recent and added what I had for tests so far. As I was saying earlier, if you run that one spec it passes but if you run the entire suite it fails.

@shwetha-manvinkurke shwetha-manvinkurke added status: work in progress Twilio or the community is in the process of implementing and removed status: waiting for feedback waiting for feedback from the submitter labels Jul 6, 2021
@raquelxmoss
Copy link

Any updates on the progress of this PR?

@rhuberdeau
Copy link
Author

@raquelxmoss @thinkingserious I found the issue and updated the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: work in progress Twilio or the community is in the process of implementing type: bug bug in the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

to_json causes and error SystemStackError when converting Message List to JSON
4 participants