Adds a new normalizer to normalize œ to oe and æ to ae #278
Conversation
Soham1803
changed the title
| // Make a small documentation of the specialized Normalizer like below. | ||
| /// <Script/Language> specialized [`Normalizer`]. | ||
| /// | ||
| /// This Normalizer uses [`<UsedLibraryToNormalize>`] internally to normalize the provided token. | ||
| /// <OptionalAdditionnalExplanations> |
There was a problem hiding this comment.
Can you please modify the documentation in order to explain the implementation, please?
| // Include the newly implemented Normalizer in the tokenization pipeline: | ||
| // - change the name of the file `dummy_example.rs` to `dummy.rs` | ||
| // - import module by adding `mod dummy;` (filename) in `normalizer/mod.rs` | ||
| // - Add Normalizer in `NORMALIZERS` in `normalizer/mod.rs` | ||
| // - check if it didn't break any test or benhchmark |
There was a problem hiding this comment.
The normalizer is implemented, however, you didn't import it in the mod.rs file and added in NORMALIZERS, you should follow the bellow instructions and remove them from the file:
| // Include the newly implemented Normalizer in the tokenization pipeline: | |
| // - change the name of the file `dummy_example.rs` to `dummy.rs` | |
| // - import module by adding `mod dummy;` (filename) in `normalizer/mod.rs` | |
| // - Add Normalizer in `NORMALIZERS` in `normalizer/mod.rs` | |
| // - check if it didn't break any test or benhchmark |
There was a problem hiding this comment.
Sure, just want to confirm one thing, should it be added in NORMALIZERS or LOSSY_NORMALIZERS?
There was a problem hiding this comment.
Hello @Soham1803,
deeply sorry for the delay,
LOSSY_NORMALIZER is better
|
Hello @Soham1803, let me know if you need more informations |
|
Hey @ManyTheFish I have made the requested changes can you please check and let me know. Thank You! |
278: Adds a new normalizer to normalize œ to oe and æ to ae r=ManyTheFish a=Soham1803 # Pull Request ## Related issue Fixes #268 ## What does this PR do? - Creates a new normalizer *ae_oe_normalizer* - normalizes `œ` and `Œ` to `oe`, `æ` and `Æ` to `ae`. ## PR checklist Please check if your PR fulfills the following requirements: - [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [x] Have you read the contributing guidelines? - [x] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! Co-authored-by: Soham <[email protected]>
|
Hello @Soham1803, The tests don't seem to work. Could you fix them? Thank you |
|
Build failed:
|
|
Hey @ManyTheFish, I'm trying to run the tests on my local machine but got some issues with |
|
Hey @ManyTheFish sorry for the delay. Thanks for your suggested changes. The last commit passed all the tests on local successfully, after many different tryouts. Made some major changes to the Normalizer. Suggest me any changes if necessary before the merge. Thank You! |
|
Hello @Soham1803 @ManyTheFish is on Holidays and will review your PR when coming back In the meantime, can you fix the Rustfmt tests? 😊 Thanks again for your PR |
|
Hello @curquiza, dealt with the Rustfmt tests. I guess all tests are passed. Now, just waiting for @ManyTheFish to suggest any required changes before the merge. Thank You! 😊 |
There was a problem hiding this comment.
LGTM!
Thank you @Soham1803 for your work. This should be part of the Meilisearch v1.9.0 release!
bors merge
|
Build succeeded: |
|
Nice to have you back @ManyTheFish. Thanks for accepting my PR and the help you provided. 😊 |
Pull Request
Related issue
Fixes #268
What does this PR do?
œandŒtooe,æandÆtoae.PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!