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

Improvements to the push module #1364

Merged
merged 9 commits into from Nov 9, 2022
Merged

Improvements to the push module #1364

merged 9 commits into from Nov 9, 2022

Conversation

zecakeh
Copy link
Contributor

@zecakeh zecakeh commented Oct 31, 2022

See these spec PRs for the last commit's logic: matrix-org/matrix-spec#1319, matrix-org/matrix-spec#1320.

Closes #466.
Closes #467.


Preview Removed

@zecakeh zecakeh requested a review from a team October 31, 2022 11:20
@zecakeh zecakeh removed the request for review from a team October 31, 2022 13:29
@zecakeh zecakeh marked this pull request as draft October 31, 2022 13:30
@zecakeh zecakeh marked this pull request as ready for review November 3, 2022 15:24
@zecakeh zecakeh requested a review from a team November 3, 2022 15:26
@zecakeh
Copy link
Contributor Author

zecakeh commented Nov 3, 2022

I'm also planning to change the client-api's set_pushrule::Request type to make it easier to construct according to the RuleKind, and also have a method to construct an AnyPushRule from it.

@zecakeh
Copy link
Contributor Author

zecakeh commented Nov 4, 2022

The change in the client-api probably makes more sense to be done before the latest commit. As it's the request's rule type that we want to insert in a Ruleset, not AnyPushRule.

@zecakeh zecakeh marked this pull request as ready for review November 4, 2022 17:43
@zecakeh zecakeh requested a review from a team November 4, 2022 17:43
@zecakeh
Copy link
Contributor Author

zecakeh commented Nov 4, 2022

Since there's a lot of commits, this PR can be easily reviewed commit-by-commit.

richvdh pushed a commit to matrix-org/matrix-spec that referenced this pull request Nov 8, 2022
This is based on the behavior of Synapse and Dendrite. Conduit's implementation is already non-compliant in regards to what was already defined in the spec.

Closes #645.
Related to #647 (probably closes it too, unless we want to be more explicit somewhere about what can be changed on default push rules).

Related PR in ruma that would allow to fix Conduit's implementation: ruma/ruma#1364

Signed-off-by: Kévin Commaille zecakeh@tedomum.fr
In practice, rule insertion is more complex than
adding rules at the end of the ruleset. It can be
easily replaced by using the methods of IndexSet.
@zecakeh
Copy link
Contributor Author

zecakeh commented Nov 9, 2022

Rebased to solve conflicts. The spec PRs have been merged so it should be fully ready for review.

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this!

crates/ruma-common/src/push/predefined.rs Show resolved Hide resolved
crates/ruma-common/src/push/predefined.rs Show resolved Hide resolved
crates/ruma-common/src/push.rs Show resolved Hide resolved
crates/ruma-common/src/push/iter.rs Show resolved Hide resolved
crates/ruma-client-api/src/push.rs Show resolved Hide resolved
crates/ruma-common/CHANGELOG.md Outdated Show resolved Hide resolved
crates/ruma-common/src/push.rs Outdated Show resolved Hide resolved
crates/ruma-common/src/push.rs Outdated Show resolved Hide resolved
crates/ruma-common/src/push.rs Outdated Show resolved Hide resolved
crates/ruma-common/src/push.rs Outdated Show resolved Hide resolved
@zecakeh zecakeh merged commit 05356d7 into main Nov 9, 2022
@zecakeh zecakeh deleted the push-rule branch November 9, 2022 13:45
clokep pushed a commit to clokep/matrix-spec that referenced this pull request May 3, 2023
…rix-org#1319)

This is based on the behavior of Synapse and Dendrite. Conduit's implementation is already non-compliant in regards to what was already defined in the spec.

Closes matrix-org#645.
Related to matrix-org#647 (probably closes it too, unless we want to be more explicit somewhere about what can be changed on default push rules).

Related PR in ruma that would allow to fix Conduit's implementation: ruma/ruma#1364

Signed-off-by: Kévin Commaille zecakeh@tedomum.fr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add more functionality to push Ruleset Make set_pushrule::Request an enum
2 participants