Intent decorator refactoring

[thorstenMueller]

Description:

• Some intents where placed in "de" (wrong) folder and some in "de-de" (correct) folder
  • I've merged them into the correct folder and removed "de" folder
• I got error "Failed to load skill: mycroft-homeassistant (NameError("name 'intent_handler' is not defined"))" during loading skill.
  • I fixed imports to correct location
• I refactored intent_handler decorator (previous intent_file_handler)
  • https://github.com/MycroftAI/mycroft-homeassistant/issues/48

[Tony763]

Hi, shouldn't from mycroft.skills.core import MycroftSkill stay from mycroft import MycroftSkill? I think using mycroft.skills.core was deprecated. @stratus-ss Is necessary to load intent_handler?

[thorstenMueller]

Thanks for your quick feedback @Tony763 .

I saw a code snipplet from @krisgesling and thought using mycroft.skills.core is correct, but i'm sure @krisgesling can point us to the right direction.

[Tony763]

Yop, I read it from Doc

[krisgesling]

Yep @Tony763 is right, it should just be imported from mycroft now. That should have been removed from the Weather Skill also.

@thorstenMueller I know this is a smaller PR, however there seems to be two distinct changes we're making here:

1. refactoring the intent_file decorator
2. fixing the de directories (which may already be in a PR from Tony)

It's a good practice to keep these two changes separate, at least as separate commits but they could even be separate PR's. This makes it much easier to review what's changed and why.

If you head to the "files changed" tab of this PR, by default it will show all changes, but you can also go through each commit one by one. Particularly for larger pull requests this is how we often review the changes. So if those changes are separated into their own commits it's easier to see that all the changes are indeed achieving what's in the commit message.

Like function names, any time you are putting "and" into a commit message, it's a good time to consider whether you're trying to squeeze too much into one commit. eg "refactoring x and fixing y" will most often be better as two commits: "refactoring x" and "fixing y".

Would you be able to resubmit this as two commits? The easiest method to do this may be to revert the intent_file changes, squash all the commits together into 1, then make the refactoring changes again as a new commit. If you're not too familiar with Git, let us know if you need a hand with the commands for this.

[thorstenMueller]

Hey @Tony763 and @krisgesling . Thanks for correcting me on this.

First i'm gonna clean up this PR just for refactoring the intent_file decorator with correct mycroft import.

@Tony763 Your pending PR (https://github.com/MycroftAI/mycroft-homeassistant/pull/50/files) is updating lots of language files. I think that the "de" language folder isn't used, or? So should i make a second PR where i merge all "de" translations to the correct "de-de" folder and remove obsolete "de" folder?

[Tony763]

Hi, de folder is just removed, but lots de intens are corrected in other commit by Gador. If You could, make only PR for intents decorators refactory and wait until my PRs are finished. Theres no need for double work. :)

[thorstenMueller]

Hey @Tony763 and @krisgesling . I've rolled back any "de" language movement stuff and just changed intent handling in init.py.

[Tony763]

Hi @thorstenMueller If I could recommend You, try rebase to squash these commits into one.

In Your case it would be: git rebase -i HEAD~7

In editor set commits as:

drop cddb1a5 
drop 7ddb476
drop 8896195
drop 9aa43d3 
drop 131ef7f
drop 8163858
pick bd69d58 

It will help to keep a commits clean and nicely readable.

[thorstenMueller]

Hey @Tony763 . Thanks for guiding me through. I've run your commands from above and pushed it, but in this PR i still see lots of not needed commits. Sorry for the confusion, but i've never worked with rebasing.

[Tony763]

I am still learning things too. I forget to mention that after using git rebase you have to force update with git push -f.

[thorstenMueller]

Ups. While trying to rebase for cleaning up commit history i rolled back to many commits. And while trying to fix this, it looks like i made it worse (PR was closed someway automatically). Give me some time and i'll fix it the right way (hopefully) ;-)

[thorstenMueller]

OMG. I thought it didn't look too bad, but realized i forgot the right import of "intent_handler". It seems not to be my best day. Maybe I should drop and create a new fork.

[Tony763]

That is also a way, Your change is not that big so making a new PR with new fork is nothing big. Rebase is hard to get along but its only about Try and Error.

[thorstenMueller]

Cause it's really just a small change i'll close this pr and open a new one from a fresh fork with just the essential changes to keep commit history clean. Thanks for your help @Tony763 .