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

handle multiples character in flag parsing #109

Merged
merged 2 commits into from
Jun 12, 2024

Conversation

gwen-lg
Copy link
Contributor

@gwen-lg gwen-lg commented Jun 12, 2024

flag allow multiple characters, but only the first one was take.
In fr hunspell dictionary, (), {}, ||, -- are used as flags.
This change create a String with all characters, not only the first on.
Without this change, the french dictionary failed to load.

I have tested than en dictionary always load, but I haven't particularly tested it more.

Fixes #108

flag allow multiple characters, but only the first one was take.
In fr hunspell dictionary, `()`, `{}`, `||`, `--` are used as flags.
This change create a String with all characters, not only the first on.
Without this change, the french dictionary failed to load.
@tgross35
Copy link
Contributor

Thanks for the PR! Most of the clippy errors aren't from this (fixed in #110). Could you rebase?

Also this should have a regression test, do you mind adding something in https://github.com/pluots/zspell/blob/2deff3a2d128b94e534da3fdca1f1858fac7cc0c/zspell/src/affix/tests_parse.rs?

@tgross35 tgross35 merged commit fe17d09 into pluots:main Jun 12, 2024
12 checks passed
@tgross35
Copy link
Contributor

Nevermind, I'll add the test separately :). Thanks!

@gwen-lg
Copy link
Contributor Author

gwen-lg commented Jun 12, 2024

ok, as you want, I was looking to do it :)
Thank you as well.

@gwen-lg
Copy link
Contributor Author

gwen-lg commented Jun 12, 2024

Not sure if it's the good way to write it (I don't know enough the code), but I do this :

#[test]
fn test_line_key_parser_long() {
    let s = "NEEDAFFIX ()";
    assert_eq!(
        line_key_parser(s, "NEEDAFFIX", |val| Ok(AffixNode::AfxNeededFlag(
            val.to_owned()
        ))),
        Ok(Some((AffixNode::AfxNeededFlag("()".to_owned()), "", 0)))
    );
}

@tgross35
Copy link
Contributor

The issue is actually a bit deeper, it comes from the FLAG long directive (flags are two characters) which isn't yet tested. So I think you will probably have incorrect answers elsewhere. I am taking a look at that now.

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.

Failed to parse french hunspell dictionary
2 participants