Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Conversation

@t3chguy
Copy link
Member

@t3chguy t3chguy commented May 25, 2023

Requires #10905
Requires #11000

Split into sensible commits


This change is marked as an internal change (Task), so will not be included in the changelog.

@t3chguy t3chguy added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label May 25, 2023
@t3chguy t3chguy self-assigned this May 25, 2023
@t3chguy t3chguy marked this pull request as ready for review May 25, 2023 13:17
@t3chguy t3chguy requested a review from a team as a code owner May 25, 2023 13:17
@t3chguy t3chguy requested review from richvdh and weeman1337 and removed request for a team May 25, 2023 13:17
Base automatically changed from t3chguy/slashcommands-peg to develop May 25, 2023 15:29
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Split into sensible commits

sensible they may be, but 38, many of which are called "iterate", is too many for a plausible review

@t3chguy
Copy link
Member Author

t3chguy commented May 25, 2023

@richvdh fair, will split into 2/3 PRs

@t3chguy t3chguy marked this pull request as draft May 25, 2023 18:41
@t3chguy
Copy link
Member Author

t3chguy commented May 25, 2023

Ah, half of the commits as from the previous PR in the chain and have 0 effect and need rebasing out, first commit of this PR really is at 131e0e8

@t3chguy t3chguy force-pushed the t3chguy/types/foobar branch from f5b6fe6 to f840124 Compare May 30, 2023 12:41
@t3chguy t3chguy marked this pull request as ready for review May 30, 2023 12:59
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

// The set of device IDs we're currently displaying toasts for
private displayingToastsForDeviceIds = new Set<string>();
private running = false;
private runningClient?: MatrixClient;
Copy link
Member

Choose a reason for hiding this comment

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

tbh it feels a bit odd to conflate running and client: personally I would keep them separate, with a comment on client along the lines of "only defined when 'running' is true", even if, strictly speaking, running is then redundant.

private displayingToastsForDeviceIds = new Set<string>();
private runningClient?: MatrixClient;
private running = false;
private client?: MatrixClient;
Copy link
Member

Choose a reason for hiding this comment

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

comment please. When is it set, when is it undefined

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm

Co-authored-by: Richard van der Hoff <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

T-Task Refactoring, enabling or disabling functionality, other engineering tasks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants