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

Add convenience methods and make setters fluent #82

Merged
merged 2 commits into from Jan 15, 2019

Conversation

Projects
None yet
3 participants
@davidstanley01
Copy link
Contributor

davidstanley01 commented Dec 28, 2018

This PR does a few things in an attempt to address issue #75:

  1. Converts all setter methods to be fluent
  2. Adds type-hints where they were previously missing
  3. Adds constants and various convenience methods to prevent excessive null checks
  4. Adds new conversation classes so we can do some easier type-hinting

Making the setters fluent is a rather opinionated step, so I won't be surprised if some don't favor it. Let me know what you think. If you don't like that, then I can easily back that out and we can keep the rest.

@davidstanley01 davidstanley01 requested review from bkuhl and craig-davis Dec 28, 2018

@davidstanley01

This comment has been minimized.

Copy link
Contributor

davidstanley01 commented Dec 28, 2018

This also addresses items 1, 5, and 6 in issue #81

@@ -356,56 +371,83 @@ public function getId(): ?int
return $this->id;
}
public function setId(int $id)
public function setId(int $id): Conversation

This comment has been minimized.

@craig-davis

craig-davis Dec 28, 2018

Contributor

Does a type hint of self instead of Conversation allow this to be extended more easily?

This comment has been minimized.

@davidstanley01

davidstanley01 Dec 28, 2018

Contributor

I'm not sure how that would make it easier to extend. Can you provide an example or explain?

This comment has been minimized.

@bkuhl

This comment has been minimized.

@craig-davis

craig-davis Jan 8, 2019

Contributor

@davidstanley01 That's my question. I don't know if it makes a difference - I have tended to type hint self and then add a phpdoc comment for @return $this to indicate that it is a fluent interface instead of an immutable return. I don't know if there is a real difference between self and Conversation in this context though.

This comment has been minimized.

@davidstanley01

davidstanley01 Jan 15, 2019

Contributor

I'm going to merge this and we can come back to it later if that works for you...

@bkuhl

bkuhl approved these changes Jan 2, 2019

namespace HelpScout\Api\Conversations;
class EmailConversation extends Conversation

This comment has been minimized.

@bkuhl

bkuhl Jan 2, 2019

With the introduction of typed Conversation objects like this, a further enhancement to this would be to update fetching a list of conversations to return these typed objects rather than generic Conversation.

This isn't something I think we should hold up this PR on though, too much good stuff here.

@davidstanley01 davidstanley01 merged commit 57c25da into v2 Jan 15, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@davidstanley01 davidstanley01 deleted the convenience-methods branch Jan 15, 2019

davidstanley01 added a commit that referenced this pull request Jan 15, 2019

Use email address when customer Id not available (#80)
* Ensure that either customer ID or customer email are present

* Fix sniff errors

* Consolidate logic into trait

* Fix getFirstEmail and update test to pass email object into collection

* Add convenience methods and make setters fluent (#82)

* Add convenience methods and make setters fluent

* Add fluent setters and type-hints to customer

* Ensure that either customer ID or customer email are present

* Fix sniff errors

* Consolidate logic into trait

* Fix getFirstEmail and update test to pass email object into collection

* Update composer and re-run fix command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment