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

Prefer stable /timestamp_to_event endpoint first - MSC3030 #2915

Merged
merged 23 commits into from
Jan 9, 2023

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Nov 29, 2022

Prefer stable /timestamp_to_event endpoint first, then fallback to the unstable MSC3030 prefixed version.

MSC3030 has been merged into the spec.

Stabilized in Synapse via matrix-org/synapse#14471

Dev notes

Add console color codes

enum AnsiColorCode {
    Red = 31,
    Green = 32,
    Yellow = 33,
}
// Add color to text in the terminal output
function decorateStringWithAnsiColor(inputString: string, decorationColor: AnsiColorCode): string {
    return `\x1b[${decorationColor}m${inputString}\x1b[0m`;
}

Todo

  • Create follow-up PR to add ANSI color codes back to test assertions

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

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

@MadLittleMods MadLittleMods marked this pull request as ready for review November 29, 2022 22:51
@MadLittleMods MadLittleMods requested a review from a team as a code owner November 29, 2022 22:51
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

With my web hat on, this does need a test to ensure the correct endpoints are called in fallback scenarios (if we're going that route). Otherwise, the unstable-stable flag or version checks need to be tested.

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Overall this is looking better - thanks for adding the test! Just some points on the code itself and this should be fine to land.

spec/unit/matrix-client.spec.ts Outdated Show resolved Hide resolved
spec/unit/matrix-client.spec.ts Outdated Show resolved Hide resolved
spec/unit/matrix-client.spec.ts Outdated Show resolved Hide resolved
spec/unit/matrix-client.spec.ts Outdated Show resolved Hide resolved
spec/unit/matrix-client.spec.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

@MadLittleMods thank you for all your work on this. With the little changes I added, it looks great.

Co-authored-by: Eric Eastwood <erice@element.io>
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

otherwise lgtm - thanks for taking a look

src/client.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants