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

Add lock/unlock method to Thread class #1240

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

Conversation

BaeFell
Copy link

@BaeFell BaeFell commented Jul 6, 2024

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

@BaeFell
Copy link
Author

BaeFell commented Jul 6, 2024

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

@valzargaming
Copy link
Member

valzargaming commented Jul 6, 2024

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.

valzargaming
valzargaming previously approved these changes Jul 7, 2024
@BaeFell
Copy link
Author

BaeFell commented Jul 7, 2024

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
Copy link
Author

BaeFell commented Jul 11, 2024

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
Copy link
Collaborator

Log1x commented Jul 11, 2024

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
Copy link
Author

BaeFell commented Jul 11, 2024

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
Copy link
Collaborator

Log1x commented Jul 11, 2024

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
Copy link
Member

SQKo commented Jul 21, 2024

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)

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