Skip to content
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

feat : string possibility with interact #1199

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Refaltor77
Copy link
Contributor

Subtlety, but which allows a tremendous time-saving on large bot projects with this API :)

Comment on lines +265 to +269





Copy link

Choose a reason for hiding this comment

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

why ?

Comment on lines +360 to +376
* Rename channel
*
* @param string $name
* @return ExtendedPromiseInterface
*/
public function rename(string $name): ExtendedPromiseInterface
{
if ($this->name === $name) {
return reject(new InvalidOverwriteException('The name is already set in the channel.'));
}

$this->name = $name;
return $this->http->patch(Endpoint::bind(Endpoint::CHANNEL, $this->id),[
'name' => $this->name
]);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be in a separate PR

Copy link
Member

Choose a reason for hiding this comment

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

I disagree with this, there is already $channel->name setter and save()
we cant let adding every function for changing every simple attribute like this

people would make multiple different http calls just to update a channel information

Copy link
Member

@SQKo SQKo left a comment

Choose a reason for hiding this comment

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

please uncommit the rename() method, besides the reasons i mentioned in the comment above, it is also out of the PR scope

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.

None yet

4 participants