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 more specific errors to CopyModerators command #196

Merged
merged 3 commits into from
Nov 9, 2023

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Nov 7, 2023

Part of the effort to improve error messages. I hate that it is a pile of try/catch but I wasn't sure how else to give more specific information about where things are failing. Suggestions welcome.

@H-Shay H-Shay requested a review from a team as a code owner November 7, 2023 20:17
@@ -61,7 +61,7 @@ export class HelpCommand implements ICommand {
"<pre><code>" +
"!conference inviteme &lt;room&gt; - Asks the bot to invite you to the given room.\n" +
"!conference inviteto &lt;room&gt; &lt;user&gt; - Asks the bot to invite the given user to the given room.\n" +
"!conference join &lt;room&gt; - Makes the bot join the given room.\n"
"!conference join &lt;room&gt; - Makes the bot join the given room.\n" +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just a typo I noticed that was causing the last two help commands to not be shown in the client. Drive-by cleanup.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hate how JavaScript is happy to have these implied semicolons lead to effectively empty / ignored statements. Thanks for catching it :-)

Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Errors are good, just wanted to call out some good practice stuff.

src/commands/CopyModeratorsCommand.ts Outdated Show resolved Hide resolved
const fromPl: {"users"?: Record<string, any>} = await this.client.getRoomStateEvent(fromRoomId, "m.room.power_levels", "");
let toPl = await this.client.getRoomStateEvent(toRoomId, "m.room.power_levels", "");

let fromPl: { "users"?: Record<string, any> } = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

PowerLevelsEventContent may also work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does - much nicer, thanks!

Comment on lines 40 to 42

try {
var toPl = await this.client.getRoomStateEvent(toRoomId, "m.room.power_levels", "");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
try {
var toPl = await this.client.getRoomStateEvent(toRoomId, "m.room.power_levels", "");
let toPl: PowerLevelsEventContent;
try {
toPl = await this.client.getRoomStateEvent(toRoomId, "m.room.power_levels", "");

Avoid using var if you can, it doesn't conform to block scoping and tends to surprise you. In this case TypeScript should let you define toPl outside and it will always be set, because you are throwing.

src/commands/CopyModeratorsCommand.ts Outdated Show resolved Hide resolved
@H-Shay H-Shay requested a review from Half-Shot November 8, 2023 22:57
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Terrific. Thanks!

@H-Shay H-Shay merged commit d17d32f into main Nov 9, 2023
@H-Shay H-Shay deleted the shay/logging_copy_mod branch November 9, 2023 16:17
@H-Shay
Copy link
Contributor Author

H-Shay commented Nov 9, 2023

Merged #196 into main, thanks for the review!

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.

3 participants