Hello, @maxbachmann, thank you for helping with the Mycroft project! We welcome everyone
into the community and greatly appreciate your help as we work to build an AI
for Everyone.

To protect yourself, the project, and users of Mycroft technologies we require
a Contributor Licensing Agreement (CLA) before accepting any code
contribution. This agreement makes it crystal clear that along with your
code you are offering a license to use it within the confines of this project.
You retain ownership of the code, this is just a license.

Please visit https://mycroft.ai/cla to initiate this one-time signing. Thank
you!

Thanks @maxbachmann It does look good. Could you add a couple of testcases for the english half past and quarter past (in test/unittests/util/test_format.py)

Maybe @krisgesling or @chrisveilleux who are native english speakers can do the final review on the changes so they're all ok. (I'm unsure about using 12 for midnight for example but I'm used to 24 hour clocks)

@forslund sure I can add testcases for the english part later.

on the 12 hour times I was unsure aswell.
I guess in german I would usually say:
(halb|dreiviertel|halb|viertel nach) 12
but
0 Uhr zwanzig (or even 10 vor halb eins)

however using 0 gets kind of weird when using am/pm
0 Uhr zwanzig nachts makes not much sence, since 0 is a taken from the 24 hour clock

For comparision: https://github.com/project-alice-powered-by-snips/ProjectAliceModules/blob/master/PublishedModules/Psychokiller1888/DateDayTimeYear/DateDayTimeYear.py is how I currently implemented this for ProjectAlice (without am/pm support right now)

Haven't reviewed yet, but 12 is a normal way of talking about midnight including:

5 to 12
half past 12
12 midnight
12 o'clock tonight
quarter to 12

Thanks for verifying @krisgesling, always good to have the input of someone who speaks the language everyday. I don't quite trust my own skills with the language.

@maxbachman I'll wait for the extra test cases, after that we can merge this.

Many thanks for contributing!

I added unittests however I am not sure on two things:

since I already cleaned up the unicode strings and file encoding for the files I updated, I did this globally (should this be in the same PR?)
Another thing I wondered is why does /doc/conf.py enforce python2? It thought the project dropped python2 support

It'd be better if it was in a separate PR but this is fine.

doc/conf.py is not used as a main file so it doesn't really matter but it seems like it was a change we missed when upgrading to python 3. Or possibly the read the docs environment still uses python2 I'll need to verify that.

Can you join your English tests with the rest of the english tests in test_format.py?

@forslund oh I did not realise you already had english test cases. I added them to the correct file.

The python2 cleanup changes can be found in the #2396 now

Is the beahviour I described in my code comment correct? It appears to me, that it is wrong in one of the two languages

should the same PR be created for lingua_franca?

Thank you.

Hmm, the number of places thing? from thinking about it briefly the english looks wrong...But reading the docstring in the format_en.py file says:

places(int): maximum decimal places to speak

which makes the english version seem to follow the specification. On the other hand the entry function in format.py doesn't document that argument so technically anything goes but it should be specified to something...

We could raise a separate issue for that.

I think this is a decent state now. I'll do a final look over tomorrow morning and merge it.

I added an issue on the topic: https://github.com/MycroftAI/mycroft-core/issues/2397

Changes looks good! Thanks for rebasing and providing a nice clean update! Merging

And yes, I think you should create the same PR for lingua franca, the intention is to switch over to using that instead of the internal utils in the future.