stevenleeg / geemusic

A bridge between Google Music and Amazon's Alexa
GNU General Public License v3.0
664 stars 181 forks source link

YAML-ize PR #176 #179

Closed fergyfresh closed 6 years ago

fergyfresh commented 6 years ago

Making #176 YAML-ized isn't as bad as I thought, with the help of @digiltd and his wicked awesome literature links I figured out how to do it. I should be able to finish the YAML-ized #176 tonight. I will also try to pair down duplicate strings per a suggestion from @yamitzky. Great work everyone on the support! This should be so sexy when it's done, you can already see in selection.py how much cleaner it is going to be. A couple flat files that are very clear and we can use variables instead of numbers {{ song }} by {{ artist }} instead of {0} by {1} and it takes the hideous dictionary out of the equation!!!

fergyfresh commented 6 years ago

Great work has been completed thanks to all of you fine gentlemen. Keep pushing forward and Make YAML Great Again.

@stevenleeg this is good for a review. I spot checked the features I use and they are working.

Feel free to try to poke holes in this but we are good.

Now for @yamitzky and @simonzu you'll have to have a ja.yaml and a de.yaml and update the Optional Language section of the README to include your languages as reference.

And finally @digiltd thank you so much for pushing me to be great. I don't even know you really, but you've been a great friend and mentor so far!

yamitzky commented 6 years ago

I implemented Japanese responses. I will offer a PR after this PR is merged. https://github.com/yamitzky/geemusic/blob/japanese_yaml/geemusic/templates/ja.yaml

fergyfresh commented 6 years ago

PERFECT! @yamitzky that is probably what will happen, but I'm not a maintainer of this repo. If you don't mind when submit the PR with this also update the README to have ja spelt out as an option in the (Optional) Language support section if you don't mind.

# English 
LANGUAGE=en

# Japanese
LANGUAGE=ja
simonszu commented 6 years ago

I will add the YAMLized de strings after this PR has been merged ;)

fergyfresh commented 6 years ago

@stevenleeg sorry to be annoying, but this is ready and we have people that could do the German and Japanese translations. They are simply waiting for this PR to get merged before they submit a pull request.

pquerner commented 6 years ago

To make this PR better I suggest you create a "echo" function. So in case you want to switch from YML to XML some day, you only have to change 1 line instead of all language output occurrences.

simonszu commented 6 years ago

To make this PR better, i suggest to merge it first into master, so that @fergyfresh shouldn't implement it a 3rd (4th?) time. Also, why should one switch to XML?

pquerner commented 6 years ago

YML tends to be hideous for some people. XML is proven for that stuff. Also: XML was just an example. My point is, that something like language should be interfaced, so it can be switched to another system. Its code smell in my opinion.

simonszu commented 6 years ago

So, just use https://onlinexmltools.com/convert-xml-to-yaml then.

fergyfresh commented 6 years ago

How is YAML hideous? This is pretty sexy. render_template can be used with YAML and XML.

All you'd have to do is change the YAML file path to an XML file path that implements the same phrases and it would just work.

I literally rewrote this from a code smell implementation to exactly what you're talking about @pquerner. Not to be sassy, but I don't think you looked at the PR at all.

pquerner commented 6 years ago

Please, for the love of god. Leave out YML, leave out XML. That was an example.

I was just saying, that its bad to use the same echo function inside the whole project, when you can have 1 function which does the job for you. In case (!!!) you want to switch someday, you dont have to be all over the place replacecing ALL occurences you make to language stuff, you'd only have to change ONE place.

Its the same stuff in TYPO3, or any other framework. Why'd they interface that shit away? Cause it CAN CHANGE.

Its just me pointing out this one about languages, and I took the change from YML to XML as an example. Not that you should change from YML to XML, or INI or TXT, or who knows what.

I hope I made myself clear. I won't care if you change that or not. I just think thats a good change. Its your PR and I'm not the maintainer. This is my last comment about this.

fergyfresh commented 6 years ago

The only way we could go further way we could go with this is wrapping render_template, but I don't think that's gonna really buy us anything right here, right now.

pquerner commented 6 years ago

It's still not clear you understand what I mean. Say you want to switch func render_template (which comes from flask) for whatever reasons. You'd have to change all occurrences of this. If you were to have a "echo" function, you only have to change 1 occurrence. Sorry if I was rude perhaps. I didn't mean it like that if you feel that way.

fergyfresh commented 6 years ago

I'm not a huge fan of wrapping something that comes from a basic library needed for the app. But that's just my $0.02. Where do you stop, you know?

If you don't like this implementation, you should see my hacky first cut at it 9cf3ebb, with like a huge nested dictionary.

pquerner commented 6 years ago

The question is not "where do you stop", its "how to make it beautiful". ;)

stevenleeg commented 6 years ago

Sorry for the delay on this one! I was out of the country and didn't have access to a good internet connection. Really great work here, everyone. Excited to see this come together 😄