-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
refactor: enforce single param on sending/editing methods #5758
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a reason for this to be done, but either way the docs are wrong
I like this change, it had been in the back of my mind for a while. I'd argue that we should do the same thing to |
Yes same, refer to internals, I got the go from Crawl to refactor the rest, hence why I changed the title and converted to draft |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-refactored again, comment dismissed-
e7bc965
to
c469257
Compare
I think this would call for a refactor of |
Actually probably not as I am currently refactoring to what the maintainers have internally decided upon, crawl laid it out in details in #5771 |
I did read the linked issue, it suggests only content should ever be provided standalone. meaning all the checks in Something for the maintainers to address, should we still be able to pass an APIMessage to these methods still? I think it can be quite useful, but not sure if its wanted. You can also just pass options to |
Yeah I got a go on what to do, it's been discussed to pretty much do what the issue says and merge it into one param, and yeah I saw that the apimessage creation param is optional, will adress that :) |
c469257
to
572daae
Compare
Co-authored-by: SpaceEEC <[email protected]>
Co-authored-by: SpaceEEC <[email protected]>
Co-authored-by: SpaceEEC <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like I missed two:
Co-authored-by: SpaceEEC <[email protected]>
Co-authored-by: SpaceEEC <[email protected]>
Some examples were not changed : discord.js/src/structures/interfaces/TextBasedChannel.js Lines 146 to 158 in 7b2e12b
I saw another but it got patched while I was writing this comment lol |
same here : discord.js/src/structures/interfaces/InteractionResponses.js Lines 167 to 170 in 7b2e12b
|
Are you able to open a PR fixing those docs, @austriandev? If so, it'd be nice if you could work on those, otherwise let us know and a contributor will work on those 👍🏼 |
Ok, I will |
Please describe the changes this PR makes and why it should be merged:
This PR refactors the methods across the the lib's sending/editing methods to avoid inconsistent overloads by removing multiple param options and enforcing single param usage as laid out in #5771.
(and let's pretend that there isn't a typo in the branch name)
Status and versioning classification: