Skip to content

Conversation

@ATKlegend
Copy link

just add user entity based from the documentation. hope this is the right way to do it.

just add user entity based from the documentation. hope this is the right way to do it.
Copy link
Owner

@soccernee soccernee left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @ATKlegend! I left 2 comments requesting a change.

I probably should have clarified this, but you'll also need to write some tests for the User model! If you take a look at test/prosperworks/integration you'll find the general approach for testing.

@@ -0,0 +1,12 @@
module ProsperWorks
class User < BaseEntity
Copy link
Owner

Choose a reason for hiding this comment

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

BaseEntity has more fields than the User model needs, so this should inherit instead from Base

module ProsperWorks
class User < BaseEntity

attr_accessor :id,
Copy link
Owner

Choose a reason for hiding this comment

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

Ah, and Base already declares: attr_accessor :id, so you can remove this line

@keerin
Copy link

keerin commented Aug 11, 2017

@soccernee - Maybe you want to also have the readme updated to reflect users being a supported entity and perhaps show an example?

Was gonna give the issue a shot but ATKlegend has this :)

@soccernee
Copy link
Owner

@ATKlegend: any interest in finishing this PR? Otherwise I'm inclined to give it to @keerin.

@keerin
Copy link

keerin commented Sep 11, 2017

@soccernee @ATKlegent Just let me know. I can get this done this weekend most likely :)

@dzliang100
Copy link

Hey, I've never contributed to open source before and it doesn't look like the issue has closed; do you mind if I work on this?

@soccernee
Copy link
Owner

@dzliang100: it's all yours, go for it. Please create a separate PR, though. Thanks!

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.

4 participants