Skip to content
This repository was archived by the owner on Mar 7, 2025. It is now read-only.

Conversation

@nathantsoi
Copy link
Contributor

Allow users to send a person data JSONObject by calling setPersonData(JSONObject)

@brianr
Copy link
Member

brianr commented Aug 28, 2013

Awesome, thanks! @sbezboro, can you review?

One suggestion: might be nice to have a helper method to the effect of setPersonData(String id, String username = null, String email = null)

@nathantsoi
Copy link
Contributor Author

Good idea!
On Aug 27, 2013 9:23 PM, "Brian Rue" notifications@github.com wrote:

Awesome, thanks! @sbezboro https://github.com/sbezboro, can you review?

One suggestion: might be nice to have a helper method to the effect of setPersonData(String
id, String username = null, String email = null)


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-23390165
.

@sbezboro
Copy link
Contributor

Thanks for the PR! I feel like person data should just be set via mutators and not via the Notifier constructor though. I wanted to keep the constructor/Rollbar.init() as clean as possible as it seems more Java-like to me to set optional configuration settings after the fact, unlike some of our other libraries. Just seems like this opens up scenarios for a lot more overloaded Rollbar.init() variants for new optional settings introduced in the future, which can become unwieldy.

What do you think?

@nathantsoi
Copy link
Contributor Author

Yep, that sounds good. From a usage perspective might be more practical w/
a setter. I actually don't use the constructor, tried to at first, but
quickly realized the user info isn't available in the extended application
class.

Thought I'd leave it in the pull request if it is useful for others, but
agree the constructor overloading could get out of hand.
On Aug 28, 2013 5:00 PM, "Sergei Bezborodko" notifications@github.com
wrote:

Thanks for the PR! I feel like person data should just be set via mutators
and not via the Notifier constructor though. I wanted to keep the
constructor/Rollbar.init() as clean as possible as it seems more
Java-like to me to set optional configuration settings after the fact,
unlike some of our other libraries. Just seems like this opens up scenarios
for a lot more overloaded Rollbar.init() variants for new optional
settings introduced in the future, which can become unwieldy.

What do you think?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-23458208
.

sbezboro added a commit that referenced this pull request Aug 29, 2013
@sbezboro sbezboro merged commit a4eb8b8 into rollbar:master Aug 29, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants