search

MycroftAI / skill-weather

Mycroft AI official Weather Skill, providing weather conditions and forecasts.
https://mycroft.ai/skills
Apache License 2.0
19 stars 59 forks source link

Make weather great again #158

Closed chrisveilleux closed 3 years ago

chrisveilleux commented 3 years ago

Description

Major refactor of the skill.

Looking at this PR might hurt your eyes. Might be easier just to checkout the branch and look at the changes holistically. There are no docstrings or unit tests yet. The VK tests do pass and many of the expected fails no longer fail. Mostly putting this up to get feedback about the refactor.

Type of PR

Testing

VK tests pass. Install skill and have at it!

Documentation

As noted previously, no docstrings yet. But they will be added before merge.

forslund commented 3 years ago

I'll dig into it more later today, just curious of the source submodule. What is the meaning of source in this case?

forslund commented 3 years ago

I focused on functionality and added the minor things I saw at a quick glance. A very nice improvement over all.

chrisveilleux commented 3 years ago

I'll dig into it more later today, just curious of the source submodule. What is the meaning of source in this case?

I am not married to the name of the submodule. I have used it before for "source" code. I just wanted to name it something on the equivalent of the dialog or vocab directory.

forslund commented 3 years ago

Ok then I get what you're going for. source feels a bit weird in something that is essentially a python module. I personally would find it more logical if it was called something named than something generic. especially since the entrypoint is not in the source folder.

Sidenote: the dialog, vocab, regex folders should likely be merged into the locale folder which is the newer convention (the multifolder resource structure is deprecated)

chrisveilleux commented 3 years ago

Is there a skill I can look at that uses the the newer convention? I've already moved a bunch of dialog and vobab files around. seems like a good time to update to the latest convention.

chrisveilleux commented 3 years ago

Ok then I get what you're going for. source feels a bit weird in something that is essentially a python module. I personally would find it more logical if it was called something named than something generic. especially since the entrypoint is not in the source folder.

Sidenote: the dialog, vocab, regex folders should likely be merged into the locale folder which is the newer convention (the multifolder resource structure is deprecated)

I have never been a fan of the top level directory being a python package. There are so many files in there (README.md, requirements.txt, etc.) that are not python code. Long term, I think the __init__.py should go in the source directory as well.

forslund commented 3 years ago

The spotify skill uses the locale and the mark-2 skill as well. Though neither of them uses the subfolder system very well (updating that in spotify at the moment)

I agree that the __init__.py should be moved in to the source folder if this setup should be used. I'd probably not name it source since that's more c/c++ style than python...maybe just call it weather? But I won't get into a naming convention fight right now, any scheme is fine.

Using a subfolder for all skill code I would also move the locale folder into the subfolder (since it's a resource for the code in that folder), doing that I think the only issue would be the way we determine the skill-id...

Olzeke51 commented 3 years ago

mycroft_missng_digit heads ukp - background : not real sure how/ewhich method got 'great-again' on Mycroft ...!! having to go to work so can't do a full test... looked good at first install - THEN I did a 'Restart' from the pull down menu BUT on the 4 hour display for 'Today's Weather" -- I miss a temperature unit's digit on one of the hours - it seems random between hours - like I mentioned -- haven't really got into it FWIW I also lost my 'listening/thinking' ring of lights - - - I thin

AIIX commented 3 years ago

QML pages are missing a background all pages or "mycroft.delegates" that require a solid background must assign one:

Mycroft.Delegate {
         id: example
         skillBackgroundColorOverlay: Qt.rgba(0, 0, 0, 1) //this can be simply "black" or any hex, rgba, hsva values https://doc.qt.io/qt-5/qml-color.html
}
Olzeke51 commented 3 years ago

moved here from the standard weather-skill as this is for the 'great-again' version Good Work - I hate to even comment ; but I was asked to test it out

**Describe the comment requested 'what is next week's weather?' got the next four days on one screen the next screen (maybe 20 secs later??) showed me the last 3 days [Mon Tues Wed]

1) THEN I started hearing the verbal conditions/hi/low {don't remember the order}

for Thur >> Sunday [while Mon .. Wed on screen]

all seemed reasonable timewise except a minor point

2) a minute (59/60 secs) after I asked -- it was at the Tues verbal when it went to 'Standby Face"

and still had to report the Wed conditions x:25 asked for weeks weather x:34 4 day shows up x:46 ring lights out x:54 3 day shows up x+1:24 Standby Face

To Reproduce Steps to reproduce the behavior: ask for 'next week's weather'

Expected behavior

1)verbal should coincide with the screen display

[was delayed]

2) last screen should hold until verbal is done

MarkII-pi DEV-kit , network cable, uSDC ubuntu default latest 04-08 with the new weather skill installed - SOmehow!!!

Good Work - liking it

chrisveilleux commented 3 years ago

The spotify skill uses the locale and the mark-2 skill as well. Though neither of them uses the subfolder system very well (updating that in spotify at the moment)

I agree that the __init__.py should be moved in to the source folder if this setup should be used. I'd probably not name it source since that's more c/c++ style than python...maybe just call it weather? But I won't get into a naming convention fight right now, any scheme is fine.

Using a subfolder for all skill code I would also move the locale folder into the subfolder (since it's a resource for the code in that folder), doing that I think the only issue would be the way we determine the skill-id...

What is the subfolder system for the locale folder? Right now I just have all the files in the folder for each locale.

chrisveilleux commented 3 years ago

QML pages are missing a background all pages or "mycroft.delegates" that require a solid background must assign one:

Mycroft.Delegate {
         id: example
         skillBackgroundColorOverlay: Qt.rgba(0, 0, 0, 1) //this can be simply "black" or any hex, rgba, hsva values https://doc.qt.io/qt-5/qml-color.html
}

Why is the background necessary? Is it related to the issue where numbers don't show up sometimes?

Olzeke51 commented 3 years ago

ran a test last night on a 'clean' image, followed one method of getting 'great-again' and no missing numbers 8 'what's the weather" checks, a pull down menu/Restart with pwr cycle - still no issues

forslund commented 3 years ago

the locale folder isn't as strict as the old regex/dialog/vocab folders so you can spread the files in subfolders, so you can group the files logically instead of by type only.

For example create a folder called locale/en-us/units in which the dialog files for celcius, fahrenheit, meters per seconds etc. could be placed

chrisveilleux commented 3 years ago

What is left for this to be merged? I still need to change the WeatherDelegate to use the CardDelegate. Which I should be able to do tomorrow. That will take care of the background issue raised by Aix. Anything else I did not address?

Olzeke51 commented 3 years ago

I am confused... this is merged into 20.08 ... but we are on 21.02 ... so does it get pulled into 21.02 somewhere along the distribution path??

chrisveilleux commented 3 years ago

I am confused... this is merged into 20.08 ... but we are on 21.02 ... so does it get pulled into 21.02 somewhere along the distribution path??

This PR was generated before 21.02 existed. Should have changed the destination branch before merge but didn't. I will work with Gez to get this into 21.02.

krisgesling commented 3 years ago

I am confused... this is merged into 20.08 ... but we are on 21.02 ... so does it get pulled into 21.02 somewhere along the distribution path??

Yeah that was my bad - it has also been merged into 21.02 though and is compatible with both versions of mycroft-core.