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

added string to iso code using strum #15

Merged
merged 5 commits into from
Jun 2, 2021
Merged

Conversation

lhr0909
Copy link
Contributor

@lhr0909 lhr0909 commented Jun 1, 2021

I am trying to add a feature to dynamically add languages in lingua-node, and needed the ability to add language codes from the outside of Rust context.

I saw there is strum so I thought to add the macro into the isocode file and it works like a charm (used a regex find/replace to add this many annotations lol)! Added test make sure the features work.

Let me know if you have any questions on the PR.

Cheers!

@pemistahl
Copy link
Owner

Hello Simon,

I see why you want this. I'm ok with adding it to Lingua but the implementation can be simplified a lot. Please remove the annotations from the enum variants again. Instead, just add #[strum(ascii_case_insensitive)] to the enum declarations. And please add it to the Language enum as well so that all public enums get this feature for the purpose of consistency. After this has been done, I will merge the PR. Thank you.

@lhr0909
Copy link
Contributor Author

lhr0909 commented Jun 2, 2021

Had to update strum to 0.21.0, but it seems fine and the tests run without any errors.

Copy link
Owner

@pemistahl pemistahl left a comment

Choose a reason for hiding this comment

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

Please remove the strum attribute serialize_all = "snake_case" from the enums. It is not needed for the code to work and there are no snake case names anyway. Thank you.

@lhr0909
Copy link
Contributor Author

lhr0909 commented Jun 2, 2021

GitHub issue and PR was down a bit, so I had to put another commit. Please feel free to squash merge! If you want me to clean up on my side, I can run an interactive rebase :D

@pemistahl pemistahl merged commit 3d8cf9e into pemistahl:main Jun 2, 2021
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.

2 participants