discord-php / DiscordPHP

An API to interact with the popular messaging app Discord
MIT License
992 stars 236 forks source link

Add lock/unlock method to Thread class #1240

Open BaeFell opened 3 months ago

BaeFell commented 3 months ago

Adds ability to lock/unlock threads using the Thread class.

BaeFell commented 3 months ago

Not entirely sure how I ended up running composer run-script cs and it had quite a few changes to make.

valzargaming commented 3 months ago

Other maintainers have argued that functions like this, which aren't directly documented by Discord, make the overall library harder to maintain. My primary question when reviewing this PR (and the Thread class in general) is: why doesn't the Channel class have a similar rename method using the patch endpoint, as the Thread class does? While I don't share the opinion that such functions complicate maintenance, it does suggest that these features might be better implemented as a shared trait among similar classes. I believe this PR is appropriate because it addresses a thread-specific issue (locking and unlocking) that doesn't apply to other contexts, so unless someone objects I am 100% for merging this.

BaeFell commented 3 months ago

I'm with you having functions like this or common ones shared as a class trait is a good thing. As long as it's possible to do something and it's not completely unintended like modals having dropdowns, then having that functionality helps everyone.

BaeFell commented 2 months ago

I messed up my PR/commit 🤦 My changes to the CommandBuilder were meant to go separate. I'm awful with Git, any advice would be appreciated to solve this 🙏

Log1x commented 2 months ago

I messed up my PR/commit 🤦 My changes to the CommandBuilder were meant to go separate. I'm awful with Git, any advice would be appreciated to solve this 🙏

Undo your command builder changes here and re-push. Afterwards, checkout back to the main branch of your fork using git checkout master and then create a new branch for your command builder changes with git checkout -b fix/commandbuilder-context-description and re-commit your command builder changes.

BaeFell commented 2 months ago

I messed up my PR/commit 🤦 My changes to the CommandBuilder were meant to go separate. I'm awful with Git, any advice would be appreciated to solve this 🙏

Undo your command builder changes here and re-push. Afterwards, checkout back to the main branch of your fork using git checkout master and then create a new branch for your command builder changes with git checkout -b fix/commandbuilder-context-description and re-commit your command builder changes.

I'll give that a go tomorrow. Thank you!

Log1x commented 2 months ago

I'll give that a go tomorrow. Thank you!

I didn't realize you did this PR off of master – what I would do is close this PR and re-fork the repo ensuring you checkout a branch before doing your changes such as git checkout -b enhance/lock-unlock-thread – it'd also allow you to do your changes without running linting since as you noted it had quite a few unrelated changes.

SQKo commented 2 months ago

You should just create a new branch in your repository then cherry pick the latest commit. And then create new PR

Other maintainers have argued that functions like this, which aren't directly documented by Discord, make the overall library harder to maintain. My primary question when reviewing this PR (and the Thread class in general) is: why doesn't the Channel class have a similar rename method using the patch endpoint, as the Thread class does? While I don't share the opinion that such functions complicate maintenance, it does suggest that these features might be better implemented as a shared trait among similar classes. I believe this PR is appropriate because it addresses a thread-specific issue (locking and unlocking) that doesn't apply to other contexts, so unless someone objects I am 100% for merging this.

For case in Thread, there exists method archive() and unarchive() method, due to the fact that it can be done outside the conditions for threads->save() i,e. different permission/being owner of the thread or is using different endpoint/method.

Locked and unlocked did not have its own method because at that time you can just do $thread->lock = true then threads->save($thread)