meilisearch / charabia

Library used by Meilisearch to tokenize queries and documents
MIT License
248 stars 86 forks source link

The `chinese-normalization-pinyin` feature flag doesn't compile #290

Closed ManyTheFish closed 3 months ago

ManyTheFish commented 3 months ago

Reproduction

cargo test --verbose --features chinese-normalization-pinyin

Fix

1) Fix the CI testing the flag using cargo test --verbose --features chinese-normalization-pinyin instead of cargo test --verbose --features chinese chinese-normalization-pinyin 1) the CI should not work anymore 1) Fix the compilation issue in the normalizer

related to https://github.com/meilisearch/meilisearch/issues/4629

Soham1803 commented 3 months ago

Just to be clear, what's expected here is the new CI command i.e. cargo test --verbose --features chinese-normalization-pinyin instead of the older cargo test --verbose --features chinese chinese-normalization-pinyin which currently runs fine. Am I right? When currently we run cargo test --verbose --features chinese-normalization-pinyin total 102+9 tests run whereas we expect only one test related to chinese normalizer to run.

ManyTheFish commented 3 months ago

I made a mistake when putting cargo test --verbose --features chinese chinese-normalization-pinyin because in this case the flag chinese-normalization-pinyin is ignored, the proper command to run would be cargo test --verbose --features chinese,chinese-normalization-pinyin. But cargo test --verbose --features chinese-normalization-pinyin is sufficient. The goal is to ensure that the code under this flag is tested and works. Currently it doesn't.

Soham1803 commented 3 months ago

Hey @ManyTheFish! Waiting for your review to this above PR by @tkhshtsh0917. I actually doubt whether that's the complete solution. Because even after that if you run cargo test --verbose --features chinese-normalization-pinyin it executes all 102+9 tests which is what happens when you simply run cargo test. I might be wrong about this. I'm sorry, I'm just curious and want to know more about this project.

tkhshtsh0917 commented 3 months ago

Hi @Soham1803! I would like to properly understand your thoughts, are you arguing that we should use a trick where only the features specified in cargo test --features are tested and other features are skipped?

Like this? ref: https://stackoverflow.com/questions/77529267/how-to-run-only-tests-marked-with-a-specific-feature-and-ignoring-the-rest

ManyTheFish commented 3 months ago

Hello @Soham1803 and @tkhshtsh0917, you are right. The number of tests doesn't change; however, they are not exactly the same. In the Chinese normalizer tests the expected result change depending on the feature flag. Whit your PR, both sides will be tested.

Soham1803 commented 3 months ago

Oh yes! I get it now. Thanks @ManyTheFish for clearing my doubt and all the best @tkhshtsh0917 with your PR. 😊