Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Rename Rust evaluator event_property_contains field value_type to value too #16045

Closed
wants to merge 2 commits into from

Conversation

bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Aug 1, 2023

Following what happened in #15781.

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@bnjbvr bnjbvr requested a review from a team as a code owner August 1, 2023 16:14
@clokep clokep changed the title fix: rename Rust evaluator event_property_contains field value_type to value too Rename Rust evaluator event_property_contains field value_type to value too Aug 1, 2023
@clokep
Copy link
Contributor

clokep commented Aug 1, 2023

Does this fix something or is it just refactoring?

@@ -405,7 +405,7 @@ pub struct EventPropertyIsTypeCondition {
pub key: Cow<'static, str>,
// During serialization, the pattern_type property gets replaced with a
// pattern property of the correct value in synapse.push.clientformat.format_push_rules_for_user.
pub value_type: Cow<'static, EventMatchPatternType>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not familiar at all with this code, so I might be super wrong. Is this using an internal format that is live-converted to what the spec mandates? If so, then does the rust module use the internal format or the spec format?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it is an internal format, the code I linked to is the serialization code which converts it.

@bnjbvr
Copy link
Member Author

bnjbvr commented Aug 3, 2023

Does this fix something or is it just refactoring?

I thought I spotted an error in passing that could have been hard to figure, but sounds like I was wrong! Closing.

@bnjbvr bnjbvr closed this Aug 3, 2023
@bnjbvr bnjbvr deleted the fix-rust-value-type branch August 3, 2023 08:52
@clokep
Copy link
Contributor

clokep commented Aug 3, 2023

Does this fix something or is it just refactoring?

I thought I spotted an error in passing that could have been hard to figure, but sounds like I was wrong! Closing.

No worries! If you're seeing the foo_type over the Client-Server API definitely shout and we can look deeper into it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants