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

Czech language: initial support #105

Merged
merged 1 commit into from Apr 28, 2020
Merged

Conversation

Tony763
Copy link
Contributor

@Tony763 Tony763 commented Apr 15, 2020

Initial support. All test passed. Will need future improvments.

@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Apr 15, 2020
Copy link
Contributor

@krisgesling krisgesling left a comment

Choose a reason for hiding this comment

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

Hey Tony, this is fantastic, congratulations on such a huge effort!

I have not yet reviewed this completely, but was taking a quick look through and saw a few things.

There's some files that you probably didn't mean to edit like the travis.yml, example files etc. We'll need to remove those changes from the PR before this could be merged.

Before the final merge, it would also be good to squash some of these commits to keep the commit history clean. If you need some advice on how to do that, please let us know.

Thanks for putting in so much work on this though, it's so great to see more people across the world being able to use Mycroft in their preferred language.

lingua_franca/format.py Outdated Show resolved Hide resolved
lingua_franca/lang/parse_cz.py Outdated Show resolved Hide resolved
lingua_franca/format.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Tony763 Tony763 left a comment

Choose a reason for hiding this comment

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

Returned date_time_format.year_format back to self.year_format > didn't know why but it failed few times when I tested it before. Now it works so it was related to different error.

lingua_franca/format.py Outdated Show resolved Hide resolved
@Tony763
Copy link
Contributor Author

Tony763 commented Apr 21, 2020

yes some help would be good. I replied to all your questions (dont know if i have or not to click on resolve conversation), put all missing files back, but i cant find a way to squash commits. Do I have to close this PR and make a new one with right settings?

@@ -582,7 +582,7 @@ def test_nice_duration(self):
"thirteen hours fifty three minutes twenty seconds")
self.assertEqual(nice_duration(50000, speech=False), "13:53:20")
self.assertEqual(nice_duration(500000),
"five days eighteen hours fifty three minutes twenty seconds") # nopep8
"five days eighteen hours fifty three minutes twenty seconds") # nopep8
Copy link
Contributor

Choose a reason for hiding this comment

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

On the one hand, #nopep8 makes me think this test was meant to verify that nice_duration() works with odd spaces in the input string, so this shouldn't be fixed.

On the other hand, the fact that the tokenizers work is proof positive that whitespace doesn't matter here, so #nopep8 could come out.

Either way, not really relevant to this PR, but it's the kind of thing that linters often catch automatically so you merge it anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd agree on this one. As it's the only test in the file with a #nopep8 flag I think it's intentionally there to test odd spacing.
@Tony763 can you switch this back please?

@ChanceNCounter
Copy link
Contributor

yes some help would be good. I replied to all your questions (dont know if i have or not to click on resolve conversation), put all missing files back, but i cant find a way to squash commits. Do I have to close this PR and make a new one with right settings?

Whoever merges the PR can squash it at that time. If you'd like to learn about squashing it yourself, it's git rebase that you want to investigate. I'd make a copy of your branch and use it to experiment, as most people screw up their first few rebases. However, it's a useful skill, so I'd encourage you to check it out.

Regardless, somebody else can take care of it in the GitHub interface.

@Tony763 Tony763 force-pushed the master branch 3 times, most recently from 0509001 to 5c2d81f Compare April 22, 2020 08:53
@Tony763
Copy link
Contributor Author

Tony763 commented Apr 22, 2020

@ChanceNCounter, @krisgesling - rebased

@Tony763
Copy link
Contributor Author

Tony763 commented Apr 24, 2020

Do I have to do something more?

@ChanceNCounter
Copy link
Contributor

Nope, sorry. I was trying to suss out whether that refactor is about to be merged, because it'd make about half this stuff unnecessary, and this would be the easier PR to alter.

But it doesn't look like that PR is gonna be merged today or tomorrow, so if nobody else gets to this by the time I'm at a computer (I'm on my phone) I'll re-review and potentially merge.

@ChanceNCounter
Copy link
Contributor

Well, I've looked it over, and everything seems to be in the right place. You should delete the .egg, don't wanna pull binaries, but I don't see anything else right away. I usually wait a couple hours and look again to be sure, though. Good devs don't trust themselves 😛

It also occurs to me that, not speaking Czech, I would have no way of knowing if there's an egregious spelling or grammatical error in there, or even if it's just a bunch of profanity. While you're deleting that .eggfile, we should ask @krisgesling whether Mycroft's had a solution to that in the past.

@ChanceNCounter ChanceNCounter linked an issue Apr 26, 2020 that may be closed by this pull request
@Tony763
Copy link
Contributor Author

Tony763 commented Apr 26, 2020

You should delete the .egg, don't wanna pull binaries

Removed, rebased.

@krisgesling
Copy link
Contributor

Yeah my Czech isn't great either.

I kind of expect that something isn't perfect but as you said elsewhere one of the easiest ways to find out is to get it out in peoples hands. Once we have a few Czech speakers using it, any inconsistencies should get flagged pretty quickly.

The biggest concern I guess would be someone intentionally adding inappropriate content in another language however they'd be going to a lot of effort for a very small joke, it would also likely get picked up fairly quickly by another speaker of that language, and we don't officially support other languages yet so we're not making any guarantees as to the accuracy of things.

I think this is ready to merge as soon as we revert the tiny change on the #nopep8 test 🎉

@Tony763
Copy link
Contributor Author

Tony763 commented Apr 28, 2020

Space reverted back. As You say, I know this is imperfect and that is why I named it initial. It will by easier to lure some czech devs or czech/english speaking people that can help with translation of skills. Really only few people want to translate something when they can't use it.

And You will be first with working (beta) voice assistant, because none of big players have it (Google, Amazon, Apple)

@krisgesling
Copy link
Contributor

Wow that is very cool, I didn't realise just how momentous this was!

🚀 🚀 🚀 🚀 Merging 🚀 🚀 🚀 🚀

@krisgesling krisgesling merged commit cf6ace5 into MycroftAI:master Apr 28, 2020
2 checks passed
@krisgesling
Copy link
Contributor

Also if you'd like a dedicated Czech language channel on Mycroft Chat, please let me know.

@Tony763
Copy link
Contributor Author

Tony763 commented Apr 28, 2020

Firstly I will send more PRs with finished translations I have made with @Manasovo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Czech language - initial support
4 participants