search

maknz / slack

A simple PHP package for sending messages to Slack, with a focus on ease of use and elegant syntax.
BSD 2-Clause "Simplified" License
1.17k stars 204 forks source link

Dynamic message endpoints feature and improved test coverage. Fixes #43 #44

Open Gadoma opened 8 years ago

Gadoma commented 8 years ago

Adds the possibility to change the Slack endpoint for each message and improves test code coverage. Fixes #43

maknz commented 8 years ago

@Gadoma thanks for your work here, much appreciated! I'm mostly happy with this, with just a couple of things I'd want to change:

I think we should let Message look after the endpoint, rather than falling back onto the client if it's missing. When a new message is created from the client, it should call setEndpoint with the (default) endpoint it holds. If the user wants to change this, they can call the endpoint / setEndpoint methods. If I call Slack::to("@regan")->send("hello"), then the Client will instantiate the Message with the default endpoint. If I call Slack::endpoint("http://fake.endpoint")->to("@regan")->send("hello") then it will instantiate the Message with the custom endpoint. I can of course continue to call setEndpoint / endpoint on the message later if I want to keep modifying it.

When it comes time to send the message, the client will always get the endpoint to use from the message, there won't be a fallback onto the client's endpoint. The client's endpoint would solely be used as the default for instantiating messages when no custom endpoint is provided.

That's the only fundamental change I'd want to make. Otherwise, you just need to do a pass over for code style: 2 spaces for indentation, new line at the end of files, ensure the doc-comments follow the existing style.

Appreciate the work on the test suite btw.

Gummibeer commented 8 years ago

Wouldn't it be better to have the endpoints all in config and just use endpoint("company") like the disk() for storage, connection() for db and so in? And keep all neccesary configs in an array for this endpoint - like:

'default' => 'company',
'teams' => [
  'company' => [
    'endpoint' => 'https://slack.foobar',
    'channel' => '#general',
  ],
  'private' => [
    'endpoint' => 'https://slack.barfoo',
    'channel' => '#random',
  ],
  // And so on
]
maknz commented 8 years ago

Yeah, I do like that style. It would certainly make things fluent. We could probably get away from "endpoints" and call them "teams". The Message could have a team field which is the key of the team, e.g. "yourcompany". The client can then manage looking up which endpoint needs to be used for that team.

Gadoma commented 8 years ago

Hi guys!

@maknz I updated the PR to include your suggestions about the endpoint handling and code style fixes.

@Gummibeer @maknz The idea of teams sounds interesting as it would provide an easy way of setting the message endpoint+channel details via a short and user-friendly alias (instead of 2 method calls with the full webhook url and channel name). Not sure about the wording though, maybe something like "targets" ?

Let me know what you think, cheers!

maknz commented 8 years ago

The only reason I defaulted to "teams" is that is the term Slack use for an instance of Slack, so you could have something like:

Slack::to("#channel")->send("Hello"); // the default team
Slack::team("mystartup")->to("#channel")->send("Hello"); // a non-default team
Slack::team("mystartup")->send("Hello"); // send to the default channel of the mystartup team
Slack::send("Hello"); // send to the default channel on the default team

I like the config from @Gummibeer above, the only thing I'd swap out is "endpoints" for "teams".

Totally fine with this being a breaking change, we just bump to 2.0 and provide upgrade instructions. Alternatively we could have a fallback which checks for the presence of the "teams" key -- if it's not there, check for "endpoint" and set it up as a team called "default".

Gummibeer commented 8 years ago

@Gadoma @maknz I've updated my example to include the suggested changes.

maknz commented 8 years ago

@Gadoma let me know if you want to build out the teams feature, or if you want to leave it with me. I'm happy to get started on that, and port over the improved test suite from this PR. Let me know what you think.

I'm planning a 2.0 release which it would be good to include the teams feature in.

Gadoma commented 8 years ago

@maknz I can carry on with implementing the teams feature 👌 This week I am quite busy so might not have the capacity but next week is a safe bet. As you mentioned the upcoming 2.0 release, I have a few other ideas so I will open separate issues for discussion, ok?

maknz commented 8 years ago

Sweet, have at it :+1:

maknz commented 8 years ago

@Gadoma how are you placed for this? I'm keen to make lots of progress towards a 2.0 release this weekend, which means the necessary API breaks with this PR.

More than happy to take over if you haven't the time or don't want to; I can take your lead with the test suite improvements as above, and implement the features outlined in this discussion.

Gadoma commented 8 years ago

@maknz Hey there mate! Currently I am fully occupied with my day-to-day bits and pieces and just don't have enough capacity to get on with the discussed features, even though I'd like to... I think the best option for now is for you to take over from here. Cheers!

maknz commented 8 years ago

Sweet as, I'll keep you posted.

maknz commented 8 years ago

Have opened #60. Will keep this PR open so I can easily refer to the code already written, including the improved test suites.