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

ref(normalization): Replace LightNormalization with NormalizeProcessor #2731

Merged
merged 4 commits into from
Nov 17, 2023

Conversation

iker-barriocanal
Copy link
Contributor

This PR removes light normalization in favor of using NormalizeProcessor directly, and updates all the calls to directly invoke NormalizeProcessor. There are no functional changes in this PR, just refactors. Note this PR doesn't get rid of "light normalization" as a concept entirely, as it's referenced in other parts of the codebase. That will be addressed in future PRs.

It's suggested to review commits sequentially.

#skip-changelog

@iker-barriocanal iker-barriocanal self-assigned this Nov 16, 2023
@iker-barriocanal iker-barriocanal requested a review from a team November 16, 2023 12:54
@iker-barriocanal iker-barriocanal changed the title Iker/ref/rm light norm ref(normalization): Replace LightNormalization with NormalizeProcessor Nov 16, 2023
Base automatically changed from iker/ref/norm-proc-visibility to master November 16, 2023 13:15
`LightNormalizationConfig` is no longer required by
`NormalizeProcessor`.
Removes the function `light_normalize_event` and the required config
`LightNormalizationConfig`.
This commit replaces direct calls to light normalization with invoking
the normalize processor directly.
Copy link
Contributor

@TBS1996 TBS1996 left a comment

Choose a reason for hiding this comment

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

lgtm

seems pretty straightforward, and it touches a lot of tests that all pass

nice job!

&mut event,
&mut NormalizeProcessor::new(normalization_config),
ProcessingState::root(),
)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

So we still use store processor here after the new NormalizeProcessor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We still need to run both processors in some circumstances until we unify them.

@iker-barriocanal iker-barriocanal merged commit 2f0602e into master Nov 17, 2023
20 checks passed
@iker-barriocanal iker-barriocanal deleted the iker/ref/rm-light-norm branch November 17, 2023 13:53
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