Tony763 commented 4 years ago

Description:

Based on @kamir86 PR #36

Possible fix for issues:

13 #20 #25 #37

Language updates: DE - @Manasovo, @gador FR - @iditude NL - @Gotanius

Checklist:

[x] Tests passing
[ ] All languages translated
[ ] Rebased

Tony763 commented 4 years ago

No problem.

Tony763 commented 4 years ago

@stratus-ss I added regex file back, but it still have some error, I do not know why. Also so comments added ad description. If you need info about specific change, let me know.

Description: For issue #13

check if IP and Token is present in config and inform if missing.
inform if Token was not accepted

For issue #20

Added all standards groups
Added handler for turning on/off all entities by default group (light, switch,fan,...) not just by group created by user.

For issue #37

Correspond with issue #20
When handling groups, jumps trough all entities in group and turn them to desired state

For issue #25

Added translation to fr-fr

Languages:

Updated languages DE, FR, NL, CZ
Rewrote intents to padatious for it's easy use, Adapt miss a chance to change order of words. What makes sense in English, in Czech(and others) with just translated words makes only nonsense. With Padatious you can make sentences with correct gramma for each language. They can be even totaly different, not just translated.
added turn.all.json with all words used directly in code, better for translation
Divide turn on/off to two intents for usage in czech as we use only one word (Zapni/Vypni)
All regex files can by removed

Tony763 commented 4 years ago

@stratus-ss Conflict solved

Tony763 commented 4 years ago

Solved merge issue from PR #41

stratus-ss commented 4 years ago

I think I want this PR pulled apart. There is so much going on. For example, you have removed a bunch of the *.rx files. Its going to take a while to go through those. Which will hold up the PR.

With so many changes it means that this PR has a chance of breaking lots of things. @krisgesling what do you think? Should PRs be broken down by 'topic'? How do you recommend handling such a large and potentially impacting one?

Further information: We are currently working on implementing a new "CommonIOT" framework and in light of that, I am not sure how the cost/benefit analysis breaks down. Some of these changes look good to me and we want and need people like @Tony763 to provide community translations, its just hard to accept translations AND sweeping changes.

One of the things that helps in huge PRs like this is having tests that ensure that nothing breaks when they go through. It's on my bucket list if I ever get around to it...

Tony763 commented 4 years ago

I understood Your fear. I firstly thinked to create mor PRs for each issue, but in the end I ended with using changes from one issue to other.

I removed regex as it is not necessary due to removing all adapt intents. This PR will definitely break other languages that were not rewrote to patatious. All new intents are included in english for all not completed language.

It would be good to make re-translation and clean up before merging this.

CZ, DE, EN is finished. FR will be finished tonight. DA, ES, IT, NL, PT, SV need translation. Help would be appreciated.

I also checked CommonIOT branch, but I did not find any update by 2019 and as all automation and integrations are run by Home Assistant (with AppDaemon) I do not see much use for it.

For Tests its not an easy task. To get really trustworthy test, You would create simulator or pack HA installation to image and use it for all changes or better create some pseudo modules for HA to simulate all tasks that are mean to be handled trough Mycroft to get a same development space for all users. Problem is, that HA is still in heavy development and every two weeks there are lot of changes, so You would have to update simulator or create a new image or check if modules you created are still supported. I think third option would be best as you can create switch, light, scene, etc.. and test them with actions. Something like this would help. No need to alter test each time to developer environment (somebody use Hue, other Evelink, Zigbee, I have my custom builds working trough mqtt,..)

krisgesling commented 4 years ago

Hey, thanks for all the effort on this Tony and Stratus!

We do generally ask for PR's to be focused on a single issue as it makes it much easier to review the code, to know whether it achieves its purpose, and in the event something breaks to know what caused it. Reviewing all of this in one PR is quite a challenge. It's almost an entirely new Skill really.

I do think it would be better to break this up into a number of smaller PR's eg:

a PR that only fixes issue 13
a PR that converts Adapt intents to Padatious
etc

If it was in one PR it would at least need to be clear through the commit history what the changes were, allowing reviewers to look at individual commits. Reviewing 65 commits in this way is just as challenging.

As stratus suggested, testing really is your friend with these big changes too. Have you had a look at the Voight Kampff testing framework? If we had a strong list of utterances that were intended to work with the Skill, it's then much easier to see if a change breaks some of these utterances. Particularly when you are tweaking intents or switching intent parsers altogether. It's a bit more difficult with something like HA as it relies on the external system being available and having particular entities to control, but even for your own local development process it adds a lot of confidence.

The purpose of the Common IoT framework is to handle situations where some things exist across multiple Skills. For example we have a Wink Skill, Hue Skill, LIFX Skill, and HA Skill. If a user says turn off all lights, then this should execute not just for Home Assistant, but also any lights that exist in other Skills. The all in one nature of HA works if users do control absolutely everything from there, so can see that many won't use this functionality, but we also need a way to allow multiple skills to use the same terminology like "turn off".

In terms of some of the other specific changes:

is there a reason to have the turn vocab in a .json file? I only had a quick glance, but would it work to have this as a .voc file and use voc_match to check if it exists in the utterance. This I believe is cleaner in the code, but also more easily translatable via translate.mycroft.ai
for translations, it's unlikely you will get all languages translated immediately for a change which is why the Translate platform exists, but we need to find a better way to get translations into Skills quickly, and for non-english speaking users to be able to continue using the Skill that works for their language. This is not a trivial problem unfortunately.

Tony763 commented 4 years ago

Hi @krisgesling. I will look into voc_match and try it.

Divide this into each PR would be a challenge for me too as all 65+ commits are not in chronological order as I worked on all issues at same time. But I checked a Voight Kampff testing and it works. I have prepared first three features and they works.

Tests need a narrow environment in HA so I prepared "dummy" switch and sensor that can be stored in info file and every developer can put tem in Home assistant without need to make big changes to code or tests. With that when You run a test you test not just Mycroft response, but also actual communication and actions in HA.

HA configuration.yaml


input_boolean:
mycroft_vk_bool:
name: "Mycroft bool"
initial: off

sensor:

platform: template sensors: mycroft_vk_temp: friendly_name: "Mycroft sensor" unit_of_measurement: '°C' value_template: 12.00

switch:

platform: template switches: mycroft_vk_switch: friendly_name: "Mycroft switch" value_template: "{{ is_state('input_boolean.mycroft_vk_bool', 'on') }}" turn_on: service: input_boolean.turn_on data: entity_id: input_boolean.mycroft_vk_bool turn_off: service: input_boolean.turn_off data: entity_id: input_boolean.mycroft_vk_bool

Voight Kampff tests


# 01sensor.feature
Feature: sensor
Scenario: read sensor
Given an English speaking user
 When the user says "give me the value of Mycroft sensor please"
 Then mycroft reply should contain "12"

02turn_on.feature

Feature: turn on Scenario: turn on switch Given an English speaking user When the user says "can you turn on Mycroft switch please?" Then mycroft should reply with dialog from "homeassistant.device.on.dialog"

03turn_off.feature

Feature: turn off Scenario: turn off switch Given an English speaking user When the user says "can you turn off Mycroft switch please?" Then "homeassistant" should reply with dialog from "homeassistant.device.off.dialog"



If I create test for all intents, would it be less fearsome for you to merge this?

For translation, after this get some final polish, we can try to ask at forum to help with fast retranslation.

Tony763 commented 4 years ago

Resolved issue for #35 Add shopping list

Tony763 commented 4 years ago

@krisgesling, @stratus-ss: If you can, check my latest commit with behave tests. All are passing.

tony@htpc-imon:~$ mycroft-skill-testrunner vktest -t homeassistant.tony763
2020-08-19 22:52:04.988 | INFO     |  2773 | msm.mycroft_skills_manager | building SkillEntry objects for all skills
2020-08-19 22:52:06.930 | INFO     |  2773 | msm.mycroft_skills_manager | Best match (1.0): homeassistant.tony763 by Tony763
2020-08-19 22:52:06.989 | INFO     |  2773 | msm.mycroft_skills_manager | Best match (1.0): homeassistant.tony763 by Tony763
-------- TEST SETUP --------
extra_skills: []
platform: mycroft_mark_1
test_skills:
- homeassistant.tony763

----------------------------
Starting all mycroft-core services
Initializing...
Starting background service bus
CAUTION: The Mycroft bus is an open websocket with no built-in security
         measures.  You are responsible for protecting the local port
         8181 with a firewall as appropriate.
Starting background service skills
Starting background service audio
Starting background service voice
Starting background service enclosure
Running behave with the arguments ""
2020-08-19 22:52:07.608 | INFO     |  2954 | mycroft.messagebus.load_config:load_message_bus_config:33 | Loading message bus configs
2020-08-19 22:52:09.096 | INFO     |  2954 | mycroft.messagebus.client.client:on_open:67 | Connected
2020-08-19 22:52:10.548 | INFO     |  2954 | msm.mycroft_skills_manager | building SkillEntry objects for all skills
2020-08-19 22:52:12,139 | Voight Kampff | INFO | Waiting for messagebus connection...
2020-08-19 22:52:12,139 | Voight Kampff | INFO | Waiting for skills to be loaded...
2020-08-19 22:52:13,543 | Voight Kampff | INFO | Starting tests for sensor
Feature: sensor # features/01_sensor.feature:1

  Scenario: read sensor                                             # features/01_sensor.feature:2
    Given an English speaking user                                  # features/steps/utterance_responses.py:120 0.001s
    When the user says "give me the value of Mycroft sensor please" # features/steps/utterance_responses.py:125 0.001s
    Then mycroft reply should contain "12"                          # features/steps/utterance_responses.py:184 0.512s
2020-08-19 22:52:17,969 | Voight Kampff | INFO | Result: passed (0.51s)

2020-08-19 22:52:18,971 | Voight Kampff | INFO | Starting tests for turn on
Feature: turn on # features/02_turn_on_switch.feature:1

  Scenario: turn on switch                                                              # features/02_turn_on_switch.feature:2
    Given an English speaking user                                                      # features/steps/utterance_responses.py:120 0.000s
    When the user says "can you turn on Mycroft switch please?"                         # features/steps/utterance_responses.py:125 0.000s
    Then "homeassistant" should reply with dialog from "homeassistant.device.on.dialog" # features/steps/utterance_responses.py:135 0.246s
2020-08-19 22:52:21,023 | Voight Kampff | INFO | Result: passed (0.25s)

2020-08-19 22:52:22,025 | Voight Kampff | INFO | Starting tests for toggle
Feature: toggle # features/03_toggle_off_switch.feature:1

  Scenario: toggle off switch                                                            # features/03_toggle_off_switch.feature:2
    Given an English speaking user                                                       # features/steps/utterance_responses.py:120 0.000s
    When the user says "can you toggle Mycroft switch please?"                           # features/steps/utterance_responses.py:125 0.001s
    Then "homeassistant" should reply with dialog from "homeassistant.device.off.dialog" # features/steps/utterance_responses.py:135 0.350s
2020-08-19 22:52:24,181 | Voight Kampff | INFO | Result: passed (0.35s)

2020-08-19 22:52:25,183 | Voight Kampff | INFO | Starting tests for toggle
Feature: toggle # features/04_toggle_on_switch.feature:1

  Scenario: toggle on switch                                                            # features/04_toggle_on_switch.feature:2
    Given an English speaking user                                                      # features/steps/utterance_responses.py:120 0.000s
    When the user says "can you toggle Mycroft switch please?"                          # features/steps/utterance_responses.py:125 0.001s
    Then "homeassistant" should reply with dialog from "homeassistant.device.on.dialog" # features/steps/utterance_responses.py:135 0.248s
2020-08-19 22:52:27,237 | Voight Kampff | INFO | Result: passed (0.25s)

2020-08-19 22:52:28,239 | Voight Kampff | INFO | Starting tests for turn off
Feature: turn off # features/05_turn_off_switch.feature:1

  Scenario: turn off switch                                                              # features/05_turn_off_switch.feature:2
    Given an English speaking user                                                       # features/steps/utterance_responses.py:120 0.000s
    When the user says "can you turn off Mycroft switch please?"                         # features/steps/utterance_responses.py:125 0.001s
    Then "homeassistant" should reply with dialog from "homeassistant.device.off.dialog" # features/steps/utterance_responses.py:135 0.344s
2020-08-19 22:52:30,288 | Voight Kampff | INFO | Result: passed (0.34s)

2020-08-19 22:52:31,290 | Voight Kampff | INFO | Starting tests for turn on
Feature: turn on # features/06_turn_on_light.feature:1

  Scenario: turn on light                                                               # features/06_turn_on_light.feature:2
    Given an English speaking user                                                      # features/steps/utterance_responses.py:120 0.000s
    When the user says "can you turn on Mycroft light please?"                          # features/steps/utterance_responses.py:125 0.000s
    Then "homeassistant" should reply with dialog from "homeassistant.device.on.dialog" # features/steps/utterance_responses.py:135 0.243s
2020-08-19 22:52:33,239 | Voight Kampff | INFO | Result: passed (0.24s)

2020-08-19 22:52:34,241 | Voight Kampff | INFO | Starting tests for set brightness
Feature: set brightness # features/07_set_brightness.feature:1

  Scenario: set light brightness                                                         # features/07_set_brightness.feature:2
    Given an English speaking user                                                       # features/steps/utterance_responses.py:120 0.000s
    When the user says "set the brightness of Mycroft light to 20 percent"               # features/steps/utterance_responses.py:125 0.001s
    Then "homeassistant" should reply with dialog from "homeassistant.brightness.dimmed" # features/steps/utterance_responses.py:135 0.266s
2020-08-19 22:52:38,517 | Voight Kampff | INFO | Result: passed (0.27s)

2020-08-19 22:52:39,518 | Voight Kampff | INFO | Starting tests for increase brightness
Feature: increase brightness # features/08_increase_brightness.feature:1

  Scenario: increase light brightness                                                       # features/08_increase_brightness.feature:2
    Given an English speaking user                                                          # features/steps/utterance_responses.py:120 0.000s
    When the user says "increase the brightness of Mycroft light please"                    # features/steps/utterance_responses.py:125 0.001s
    Then "homeassistant" should reply with dialog from "homeassistant.brightness.increased" # features/steps/utterance_responses.py:135 0.319s
2020-08-19 22:52:43,346 | Voight Kampff | INFO | Result: passed (0.32s)

2020-08-19 22:52:44,347 | Voight Kampff | INFO | Starting tests for decrease brightness
Feature: decrease brightness # features/09_decrease_brightness.feature:1

  Scenario: decrease light brightness                                                       # features/09_decrease_brightness.feature:2
    Given an English speaking user                                                          # features/steps/utterance_responses.py:120 0.000s
    When the user says "decrease the brightness of Mycroft light please"                    # features/steps/utterance_responses.py:125 0.000s
    Then "homeassistant" should reply with dialog from "homeassistant.brightness.decreased" # features/steps/utterance_responses.py:135 0.271s
2020-08-19 22:52:46,122 | Voight Kampff | INFO | Result: passed (0.27s)

2020-08-19 22:52:47,124 | Voight Kampff | INFO | Starting tests for turn off
Feature: turn off # features/10_turn_off_light.feature:1

  Scenario: turn off light                                                               # features/10_turn_off_light.feature:2
    Given an English speaking user                                                       # features/steps/utterance_responses.py:120 0.000s
    When the user says "can you turn off Mycroft light please?"                          # features/steps/utterance_responses.py:125 0.001s
    Then "homeassistant" should reply with dialog from "homeassistant.device.off.dialog" # features/steps/utterance_responses.py:135 0.254s
2020-08-19 22:52:49,084 | Voight Kampff | INFO | Result: passed (0.25s)

2020-08-19 22:52:50,085 | Voight Kampff | INFO | Starting tests for automation
Feature: automation # features/11_automation.feature:1

  Scenario: triger automation                                                                    # features/11_automation.feature:2
    Given an English speaking user                                                               # features/steps/utterance_responses.py:120 0.000s
    When the user says "activate the automation mycroft tracker automation"                      # features/steps/utterance_responses.py:125 0.001s
    Then "homeassistant" should reply with dialog from "homeassistant.automation.trigger.dialog" # features/steps/utterance_responses.py:135 0.243s
2020-08-19 22:52:53,135 | Voight Kampff | INFO | Result: passed (0.24s)

2020-08-19 22:52:54,137 | Voight Kampff | INFO | Starting tests for tracker
Feature: tracker # features/12_tracker.feature:1

  Scenario: tracker                                              # features/12_tracker.feature:2
    Given an English speaking user                               # features/steps/utterance_responses.py:120 0.000s
    When the user says "give me location of the mycroft tracker" # features/steps/utterance_responses.py:125 0.001s
    Then mycroft reply should contain " home"                    # features/steps/utterance_responses.py:184 0.280s
2020-08-19 22:52:56,723 | Voight Kampff | INFO | Result: passed (0.28s)

2020-08-19 22:52:57,725 | Voight Kampff | INFO | Starting tests for set climate
Feature: set climate # features/13_set_climate.feature:1

  Scenario: set climate temperature                                                          # features/13_set_climate.feature:2
    Given an English speaking user                                                           # features/steps/utterance_responses.py:120 0.000s
    When the user says "change the mycroft climate temperature to 24 degrees"                # features/steps/utterance_responses.py:125 0.000s
    Then "homeassistant" should reply with dialog from "homeassistant.set.thermostat.dialog" # features/steps/utterance_responses.py:135 0.309s
2020-08-19 22:53:01,643 | Voight Kampff | INFO | Result: passed (0.31s)

2020-08-19 22:53:02,645 | Voight Kampff | INFO | Starting tests for shopping list
Feature: shopping list # features/14_add_item_shopping_list.feature:1

  Scenario: add item                                                                        # features/14_add_item_shopping_list.feature:2
    Given an English speaking user                                                          # features/steps/utterance_responses.py:120 0.000s
    When the user says "add bread to the shopping list"                                     # features/steps/utterance_responses.py:125 0.000s
    Then "homeassistant" should reply with dialog from "homeassistant.shopping.list.dialog" # features/steps/utterance_responses.py:135 0.306s
2020-08-19 22:53:05,358 | Voight Kampff | INFO | Result: passed (0.31s)

14 features passed, 0 failed, 0 skipped
14 scenarios passed, 0 failed, 0 skipped
42 steps passed, 0 failed, 0 skipped, 0 undefined
Took 0m4.203s
Stopping all mycroft-core services
Stopping skills (2944)...stopped.
Stopping audio (2947)...stopped.
Stopping speech (2950)...stopped.
Stopping enclosure (2953)...stopped.
Stopping messagebus.service (2941)...stopped.

Tony763 commented 4 years ago

Used voc_match as suggested instead of a json file.

krisgesling commented 4 years ago

Hey Tony, thanks for the tests they look great! Also glad the voc_match was helpful.

I'm still pretty hesitant to merge this just because it's so hard to review. We're in the middle of releasing mycroft-core 20.08 so I can't dedicate the time to it right now, I'll also be away next week, but can get back to it the week after.

What I'd recommend is to run an interactive rebase to edit the commits in the history. To do this you'd run:

git rebase -i HEAD~69

The HEAD~69 says that you want to look at the last 69 commits as that's how many are included in this PR.

This will then give you the option for each commit to:

reword commit messages to make them more descriptive
squash commits together eg all of Gotanius's commits called "update _____" could probably be squashed down into a single "add Dutch translations"
and a bunch of other useful things.

The tool actually provides a nice description on what each of these are when you run it. Though I would suggest creating a new branch to do this on if you aren't familiar with rebasing. It's a powerful tool, which also means you can stuff it up and you don't want to ruin all the good work you've put into it.

We really appreciate the amount of work you've put into this and it does look like great changes, especially seeing the test output. We just need to make sure we're reviewing things properly, and a cleaner commit history will also be more useful when other developers are looking back at the code changes.

krisgesling commented 4 years ago

Also if that looks too daunting, I'm happy to give it a go in the first week of September. But it is a really useful tool to learn about if you're wanting to level up your Git Fu :slightly_smiling_face:

Tony763 commented 4 years ago

Yes, it is necessary to squash these commits down. I already used rebase when I worked on lingua-franca. I will firstly ask on forum for help with translation as I do not want to break so many of languages. Today will remove all unnecessary files and we will see if community help.

Updated checklist to match goals.

Tony763 commented 4 years ago

@krisgesling, @stratus-ss Rebased, check turn_on_all-group-squash.

Now I will try to get other languages done.

marvin-w commented 3 years ago

Working great for me so far! Thanks! Hope this gets merged soon.

stratus-ss commented 3 years ago

@krisgesling do you have time to sit down and chat with me about this?

Tony763 commented 3 years ago

Hello as I wrote above, please check turn_on_all-group-squash branch, it is rebased with squashed commits so they could be better understood and also contain small fix.

Tony763 commented 3 years ago

Hello also if possible check travis-test branch

I created Travis CI test for Home Assistant - Contains Codestyle check and Behave tests Travis-ci result Codestyle and Mycroft logs Behave results

marvin-w commented 3 years ago

I like the idea of having CI in place. However, for local development it might be nice to be able to run the same set of sets in the same amount of time with the same test data and the same home assistant installation. I'd therefore suggest moving some of the code from the pipeline to i.e. a docker-compose or a bash script that can be run locally from a developer workplace manually.

However, good job!

Tony763 commented 3 years ago

Thanks, mate.

For local development, docker composer sounds me as better solution than only bash script. It will help you start with clean environment each time you process a new build. I will look into it.

marvin-w commented 3 years ago

Maybe you should do this in another PR and squash this branch again as it has over 70 commits. Also feel free to ping me, I can help.

Tony763 commented 3 years ago

turn_on_all-group-squash is successor of this PR and is rebased to something human readable :D

Travis-test is based on this PR and will not work without it.

When @krisgesling or @stratus-ss will want to begin with review, I will close this PR and send a new one with turn_on_all-group-squash or maybe I will do it sooner.

Travis-test differ only in few files so after this will be accepted I can port it and send a new proper PR.

stratus-ss commented 3 years ago

@krisgesling and I spoke about this and the consensus is that while this is an improvement from the original commit, there is still too much going on. It has just been squished down.

@krisgesling can expand on that

Tony763 commented 3 years ago

That did not put more light on this topic. @krisgesling asked to rebase and squis commits down to some reasonable number, I did. If there is something more I can rework, tell me and I will do it.

I spend some long nights on this so I would like to see it working. Thanks

gador commented 3 years ago

I must agree with @Tony763 , although a lot has been changed, this is the version I have been using since I provided the translations. The original didn't work (at all) for me, and the rework Tony did provided a really great replacement. I would love to see this one being merged, so I can start following the original repo again, and not the PR (or actually my fork, which is based on the PR). Thanks @krisgesling

marvin-w commented 3 years ago

From previous experience I understand both sides. From a maintainer view this PR is too big. It has to be broken down into smaller pieces as suggested from @krisgesling earlier. He also provided feedback on how that can be achieved. You should check his comment.

Additionally, I also get the other view point from a contributor point of view. If it takes longer than 3 months for someone to even start looking at a PR this can become kind of unpleasant. And please don't get me wrong or take this as an offense, you're doing a great job here and you all probably have a lot of other stuff to do as well but that's just how it feels from a contributor view - at least for me when I had the same experience in another project unrelated to this one.

Anyway, I do appreciate all the work that has been put into this, not only this PR but this project as a whole, so thank you for that!

@Tony763 If you need help with splitting it up into smaller chunks I'd be down to help you. Just ping me.

Tony763 commented 3 years ago

Hi Marvin, I read krisgesling post carefully, last one from 20 Aug was to rebase and squash commits that makes a one change, together and add some description. That should be done. Even if you broke this into pieces it still will be lot of work to review and will not help much as all these pieces follow each other and do not work separate. Even translations than could be take apart will not work on their own as they are already made to rewrote intents.

From 71 horrible commits I toke them down to 23 and from it 3 are resolves of already merged PRs. Other 20 have nice naming and some description. If there is need to describe something more, just tell me and I will add it. If I continue with squashing more then it would lose splits between changes.

I now this is really big and there is lot of fear but I try to prove that it works well. I run behave tests across real HA installation and all are passing - described in post upward. @gador run it for long period and he reported no issue up until now Also all people from affected issues reported no problems up until now.

Breaking in this is language suport for DA,ES,IT,NL,PT,SV, but its nothing I can overcome as always if you add something new, you can't include it in all languages if you do not want to use translator.

I laso asked in forum for help with translation, it contain only few lines of text (it is not novel) but yet only guys from DE helped. That is sad a little.

In lingua- franca there is a big refactor and @krisgesling have to deal with it, I appreciate it as there are also improvements for my implementation of CS language handling. Kris take your time as that change is more important. Just let me know if I can improve something or some time schedule.

It does not mater if it takes 3 days or 3 months, it wort if it will be accepted.

@stratus-ss all changes are together, if you need some detailed information let me know. My question is, do we have some beta channel or something so the change can be pushed to users that want to test things. Also is there a way to push new texts back to translation portal?

stratus-ss commented 3 years ago

@Tony763 I will prioritize this with @krisgesling this week. I have a shortened work week which should help push this along. I want to stress that I have been where you are, and I absolutely recognize the amount of care and work that has gone into this PR.

In terms of the 3 months lag between submission and Kris' first comment, I'll take the blame there. Its part of my tasks to monitor the repo and pick off/approve the simpler changes and then work together with Kris on these bigger ones. I simply missed this PR. No excuses. I'll try and do better from here on out.

With such a big commit and the potential ramifications of these changes I cannot approve these myself. I will make some recommendation and the Mycroft team will have to vote internally on it.

I'll get back to you on your specific questions about specific tags to test against once the new work week starts

Tony763 commented 3 years ago

@stratus-ss do not blame yourself, time does not mater :) This is opensource and we all contribute to it with know that most of us do this in our free time and things can take some time.

Tony763 commented 3 years ago

@stratus-ss #36 this one is also included (my is based on it) in my PR s if kamir will not respond, it can be done by this if it will be accepted :D

Also would you like travis test for HA? Check it out here. If You will be interested, I can start to prepare it to some better form. It would help with testing and proving things.

krisgesling commented 3 years ago

Hey, it does look like a lot of great changes. It's also been a fair while since the Skill got much attention so I think part of the challenge is updating the existing code with newer practices and there's some good progress there like shifting away from manually loading files.

There are a few things I'm hoping we can work toward too (and this is 100% from the existing code, not your PR).

More descriptive variable names. It makes things so hard to review when we are talking about something like brightness_req. Is this a request, a requirement or something else starting with "req".
Better inline documentation, including docstrings on each of the methods describing their purpose, expected arguments and return values.
Splitting code out where it improves readability. The check_availability method is a great example.

The rebased branch is certainly better in terms of grouping and describing changes. Though I know it's hard once you are working with a series of commits because you are limited by how you can group them. For example the "Padatious intents" commit does change intents from Adapt to Padatious but it also changes logic around how we calculate minimum and maximum brightness values along with a few other bits and pieces.

You can do this as a single big PR, but if you are reworking it, this might work better as a series of smaller PR's. For example, adding the shopping list functionality feels completely separate to the rest of the PR and from a quick glance doesn't rely on any of the other changes. So this would be a great candidate for being split out to it's own pull request. I'd say the same for things like group handling and the min/max brightness changes. Reviewing 3 distinct changes and being able to test each one independently is much quicker and easier than those 3 changes being submitted as one.

Finally if there are languages without translations we should leave those files out, not put English in them. These will get pulled into translate.mycroft.ai and the contributors there will work through them.

As I said, it looks like lots of good changes here, and it's great to see the Skill getting the attention it needs. Thanks heaps for all your work on it! I'm hoping we can get these changes included soon.

Tony763 commented 3 years ago

Ok, I gave up. Another merging issue present and another is on the way (by me #49 ) . I will start from beginning and dig all commits apart.

Just let this PR open as placeholder, so everybody contributing see, which changes are on the way.

Tony763 commented 3 years ago

First batch divided, next will come when #52 will be merged.

Tony763 commented 2 years ago

All changes were split and integrated to HA skill. :tada:

MycroftAI / skill-homeassistant

Few updates, czech support,.. #38

Description:

13 #20 #25 #37

Checklist:

02turn_on.feature

03turn_off.feature