Padatious intents - Githubissues

MycroftAI / skill-homeassistant

Mycroft Skill/Integration for Homeassistant

GNU Lesser General Public License v3.0

114 stars 62 forks source link

Padatious intents #53

Closed Tony763 closed 3 years ago

Tony763 commented 3 years ago

Rewrote all intents to padatious
Fix in automation intent
Address PR #38

Description:

Checklist:

[ ] Code is Python 3.x compatible
[ ] Travis is passing tests Your PR cannot be merged unless tests pass
[ ] No PEP Bot Errors or Issues

Tony763 commented 3 years ago

Updated with upstream, changed new loggers to self.log

Tony763 commented 3 years ago

Solved merge conflicts. @stratus-ss Any progress?

stratus-ss commented 3 years ago

@Tony763 there are some merge conflicts now because of an update to the translations. Sorry for delaying this one

Tony763 commented 3 years ago

No problem, I will check it tonight.

Tony763 commented 3 years ago

@stratus-ss: Updated, but please try it, my Mycroft went down after Ubuntu upgrade so I cannot test it right now. Hopefully I will manage to make it working in next 2-3 days so I will test it as well.

Tony763 commented 3 years ago

Added Fr-Fr Padatious intents, thanks goes to @antipiot

Tony763 commented 3 years ago

@stratus-ss: Hi, any progress?

Tony763 commented 3 years ago

Address also issue #66.

stratus-ss commented 3 years ago

@Tony763 thanks for bumping this. I had lost track of it. I am going to go back over this on my Mark II and see how it goes

Tony763 commented 3 years ago

@stratus-ss No problem, let me know, if I should rebase this to something understandable. After these PRs are done, I will start my work on Issue #64. I have already some vision on mind.

stratus-ss commented 3 years ago

@Tony763 I am just poking at the automated testing.

Have you ran the testrunner on your own?

mycroft-skill-testrunner /opt/mycroft/skills/homeassistant.mycroftai/

For reference it runs and everything passes. I am just trying to spread awareness

stratus-ss commented 3 years ago

I think that it would be a good time to add some tests for the methods that are being changed.

I know that you are rewriting them, but the original author(s) didn't have tests that I can see for at least some of these. Let's use this opportunity to tighten that up.

Feel free to hit up @krisgesling if you need more help with the tests in this repo. Between the two of us we can probably help to reform this

Tony763 commented 3 years ago

Hi @stratus-ss, I know about tests. If possible I do not want to add more things to this PR if its working. I do not want to end up in same state as with issue #38.

For tests I had prepared another branch. It contains all tests and also travis based build test with usage of home assistant instance running in each build, so you test thing against real static environment with out put to alluretest. It should be also portable to google actions. Link to branch Travis Allure

Please, let me know if this is OK with You and I will prepare PR.

stratus-ss commented 3 years ago

Hi @stratus-ss, I know about tests. If possible I do not want to add more things to this PR if its working. I do not want to end up in same state as with issue #38.

For tests I had prepared another branch. It contains all tests and also travis based build test with usage of home assistant instance running in each build, so you test thing against real static environment with out put to alluretest. It should be also portable to google actions. Link to branch Travis Allure

Please, let me know if this is OK with You and I will prepare PR.

I'll speak with @krisgesling about this but I think because this is such an important shift tests should be included in order to verify the changes