Skip to content
This repository has been archived by the owner on Aug 28, 2020. It is now read-only.

Fix RichMenu#run() return value type #91

Merged
merged 4 commits into from
Nov 21, 2017
Merged

Fix RichMenu#run() return value type #91

merged 4 commits into from
Nov 21, 2017

Conversation

kenany
Copy link
Contributor

@kenany kenany commented Nov 21, 2017

RichMenu#run() returns the same type as RichDisplay#run(), which is
a Promise<ReactionHandler>.

@kyranet
Copy link
Contributor

kyranet commented Nov 21, 2017

Can you merge #92 with this one? No need to make multiple PRs if you can merge them in one. If you need time to find all the bugs, let me know and I'll put a WIP label to this PR.

@kyranet kyranet self-assigned this Nov 21, 2017
@kyranet kyranet added Bug: Fixed Issues that report bugs and have been fixed. Meta: Documentation Issues and PRs related to documentation. labels Nov 21, 2017
@kenany
Copy link
Contributor Author

kenany commented Nov 21, 2017

Sure thing! I did multiple PRs since the changes are more-or-less unrelated :)

@kenany kenany mentioned this pull request Nov 21, 2017
Closed
@kyranet
Copy link
Contributor

kyranet commented Nov 21, 2017

No worries! Just trying to organize the PRs a little so they don't become a mess :)

@kenany
Copy link
Contributor Author

kenany commented Nov 21, 2017

All right, these three changes are all I have for the time being :) I can squash them together if desired.

@kyranet
Copy link
Contributor

kyranet commented Nov 21, 2017

Despite of nothing in JS being able to be private, but protected (by following the name conventions), we do use all these kinds of methods as @private. Nonetheless once tc39/proposal-class-fields gets implemented, we may update many things to make them actually private. It's just, a matter of time,

@kenany
Copy link
Contributor Author

kenany commented Nov 21, 2017

Remaining typescript errors are (using master of both discord.js and klasa):

node_modules/klasa/typings/index.d.ts(26,3): error TS2305: Module ''discord.js'' has no exported member 'OAuth2Application'.
node_modules/klasa/typings/index.d.ts(31,3): error TS2305: Module ''discord.js'' has no exported member 'Attachment'.
node_modules/klasa/typings/index.d.ts(32,3): error TS2305: Module ''discord.js'' has no exported member 'RichEmbed'.
node_modules/klasa/typings/index.d.ts(33,3): error TS2305: Module ''discord.js'' has no exported member 'RichEmbedOptions'.
node_modules/klasa/typings/index.d.ts(178,29): error TS8020: JSDoc types can only be used inside documentation comments.

Dunno if they're worth fixing, since they're not based on a released discord.js version yet. (Also haven't yet figured out the last one regarding JSDoc 🤔 )

@MrJacz
Copy link
Member

MrJacz commented Nov 21, 2017

I've noticed these errors and stated this to Kyra... problem is the discord.js@master typings are for stable so the Discord.js errors are bound to happen until they're updated and the JSDoc I couldn't work out either.

@kyranet
Copy link
Contributor

kyranet commented Nov 21, 2017

At the moment of writting the typings, only 11.2.0 typings were available, and I had to work with them (RichEmbed instead of EmbedMessage, and so on). If MessageEmbed is now available, then I'd be happy if you could update them 👍

@MrJacz
Copy link
Member

MrJacz commented Nov 21, 2017

I've heard the Discord.js typings have been updated but I have not checked for myself

@bdistin
Copy link
Contributor

bdistin commented Nov 21, 2017

Can you also allow edits from maintainers. This will not be merged if this branch is out-of-date with master, and without permission, I cannot automatically merge the branch to let CI re-run.

@kenany
Copy link
Contributor Author

kenany commented Nov 21, 2017

@bdistin Enabled.

@bdistin
Copy link
Contributor

bdistin commented Nov 21, 2017

Ok, I have been discussing protected vs private with some of the others. I pulled your branch and built docs. jsdocs#protected makes those methods show up on the docs site. I don't want them to be that public, however, I am fine with the typescript definitions to refer to them as protected so that you can properly extend them with TS.

So, can we revert the jsdoc references to private, and we can keep the ts definitions as protected?

@kenany
Fix RichMenu#run() return value type
c751b1e 
`RichMenu#run()` returns the same type as `RichDisplay#run()`, which is
a `Promise<ReactionHandler>`.
@kenany
Swap _determineEmojis() signatures
e858b88 
From the code, it seems like `RichDisplay#_determineEmojis()` has the
four-argument signature, whereas `RichMenu#_determineEmojis()` has the
two-argument signature. The typings have them mixed up, so this patch
swaps the signatures to reflect the code.
@kenany
Make _determineEmojis() a protected method
a61563f 
Because `RichMenu` uses its parent's (`RichDisplay`)
`_determineEmojis()` method, the method must be defined as protected.
@kenany
Update to latest discord.js typings
f5beb2d
@kenany
Copy link
Contributor Author

kenany commented Nov 21, 2017

@kyranet Updated, PTAL.

@bdistin Reverted that change :)

@kyranet
Copy link
Contributor

kyranet commented Nov 21, 2017 •
edited
Loading

They show up as protected yet, but thanks for the typings update 👍

EDIT: I'll do a last check

bdistin
bdistin approved these changes Nov 21, 2017
View reviewed changes
Copy link
Contributor

@bdistin bdistin left a comment

Choose a reason for hiding this comment

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

Everything LGTM

kyranet
kyranet approved these changes Nov 21, 2017
View reviewed changes
Copy link
Contributor

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

LGTM

@bdistin bdistin merged commit 390c103 into dirigeants:master Nov 21, 2017
@kyranet kyranet added Meta: BugFix PRs that fix bugs or issues. and removed Bug: Fixed Issues that report bugs and have been fixed. labels Feb 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kyranet kyranet kyranet approved these changes

@bdistin bdistin bdistin approved these changes

Assignees

@kyranet kyranet

Labels
Meta: BugFix PRs that fix bugs or issues. Meta: Documentation Issues and PRs related to documentation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kenany @kyranet @MrJacz @bdistin