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

Extend Russian grammar tests #192

Merged
merged 9 commits into from
Jan 19, 2018
Merged

Conversation

yuryleb
Copy link
Contributor

@yuryleb yuryleb commented Nov 18, 2017

This PR adds test names for all status street names in Russian grammar rules and fixes some detected errors in grammar expressions.

@yuryleb yuryleb force-pushed the extend-grammar-tests branch 2 times, most recently from 148323a to 0cdb844 Compare November 27, 2017 19:44
@yuryleb yuryleb force-pushed the extend-grammar-tests branch from 0cdb844 to ea581d8 Compare December 3, 2017 20:20
@yuryleb
Copy link
Contributor Author

yuryleb commented Jan 13, 2018

Actually adding tests is infinite work but maybe somebody could review & merge this PR with made changes? All further efforts could be added with newer PRs (I have some plans to check all Russian street names collated by Russian street names validator but in some far away future)?
@1ec5, what do you think, should I update change log with these quite minor fixes (especially for other languages) or not?

@1ec5 1ec5 self-requested a review January 17, 2018 09:24
Copy link
Member

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Thanks for your patience and for your diligence with these grammar rules. I agree that we can treat this as an ongoing effort and making further improvements in future PRs.

should I update change log with these quite minor fixes (especially for other languages) or not?

Sure, if you don’t mind.

@@ -4,6 +4,8 @@
},
"v5": {
"accusative": [
["^ [«\"](.+?)[»\"] ", " трасса \"$1\" "],
Copy link
Member

Choose a reason for hiding this comment

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

The speech synthesizers I tried seem to treat guillemets equivalently to quotation marks, so this rule would be fine. On the other hand, if the instruction is presented visually (which this library doesn’t rule out), is it expected that главная аллея «Годы войны» would be presented as главная аллея "Годы войны" instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for checking this - IMHO if modern TTSes could handle different quote styles internally (Garmin's old Nuance Vocalizer Automotive doesn't understand guillemets), it's better just to keep original quotes as is.
I added this change and updated the log.

@yuryleb yuryleb force-pushed the extend-grammar-tests branch from 552e05d to 7fe63a4 Compare January 18, 2018 17:35
Copy link
Member

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Thanks again!

@1ec5 1ec5 merged commit 75a8b4c into Project-OSRM:master Jan 19, 2018
@yuryleb yuryleb deleted the extend-grammar-tests branch January 19, 2018 04:42
@1ec5
Copy link
Member

1ec5 commented Jan 19, 2018

All further efforts could be added with newer PRs (I have some plans to check all Russian street names collated by [Russian street names validator](https://github.com/AMDmi3

@yuryleb, would you mind opening separate issues describing what you’re thinking of doing in this space? We might be able to get others to help with test cases as well. Thanks!

@allierowan allierowan mentioned this pull request Jan 24, 2018
3 tasks
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