Make weather great again #158

chrisveilleux · 2021-04-06T03:29:32Z

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 · 2021-04-07T08:44:52Z

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

source/config.py

__init__.py

forslund · 2021-04-07T19:21:04Z

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

chrisveilleux · 2021-04-10T15:41:47Z

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 · 2021-04-13T15:38:46Z

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 · 2021-04-15T17:35:01Z

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 · 2021-04-16T19:55:54Z

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 · 2021-04-19T18:19:46Z

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...

__init__.py

forslund

Haven't been able to run it yet, just looked over the code and found a couple of nitpicks but nothing that really needs to be handled

__init__.py

source/weather.py

Olzeke51 · 2021-04-22T09:44:30Z

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 · 2021-04-22T13:41:47Z

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 · 2021-04-22T20:17:19Z

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 · 2021-04-23T03:02:25Z

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 · 2021-04-23T03:08:43Z

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 · 2021-04-23T10:00:01Z

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 · 2021-04-23T16:20:43Z

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

__init__.py

forslund

Found some issues regarding to the sunrise / sunset intents.

chrisveilleux · 2021-05-05T23:37:09Z

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?

forslund

The issues I found seem to have been corrected but haven't had time to re-review the code. Changing my request changes to a comment so it's not blocking if anyone gets to the reviewing before me :)

krisgesling

Loving this refactor so much!

I assure you that any comments below are far outweighed by the list of great improvements :)

__init__.py

skill/dialog.py

krisgesling · 2021-05-06T23:58:15Z

README.md

@@ -1,26 +1,47 @@
 # <img src='https://rawgithub.com/FortAwesome/Font-Awesome/master/svgs/solid/sun.svg' card_color='#FEE255' width='50' height='50' style='vertical-align:bottom'/> Weather
 Weather conditions and forecasts

+[![Code style: black](https://img.shields.io/badge/code%20style-black-000000.svg)](https://github.com/psf/black)


I'd be more comfortable leaving this out until we have made a call on what the Mycroft official requirement is.

But the code IS formatted using Black. What if someone questions why the lines are longer than 79 characters? This is informational, not prescriptive. We can always remove it.

README.md

__init__.py

krisgesling · 2021-05-07T02:25:13Z

skill/util.py

+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""Utility functions for the weather skill."""


Would be good to add some unit tests for these - they should be the simplest given they are pure.
Particularly useful as an example if we're going to point people to this as best practice.

…emoving a bunch of condition specific dialog files, code review changes and other minor cleanups.

…tract some of the grid-style card layout commonalities.

…gain # Conflicts: # ui/DailyDelegateScalable.qml # ui/highlow.qml # ui/weather.qml

krisgesling

As discussed, there is more that we want to do here but merging this version as it's been well tested IRL and will fix known bugs in the current version of the Skill.

Olzeke51 · 2021-06-14T19:53:39Z

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 · 2021-06-14T20:07:27Z

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 · 2021-06-14T21:08:15Z

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.

chrisveilleux added 2 commits February 24, 2021 20:10

Re-ordered imports and put module-level documentation where it belongs

4bff6a1

Merge branch '20.08' into make-weather-great-again

759cb1a

chrisveilleux requested review from forslund, krisgesling and dschweppe April 6, 2021 03:29

forslund reviewed Apr 7, 2021

View reviewed changes

source/config.py Outdated Show resolved Hide resolved

forslund reviewed Apr 7, 2021

View reviewed changes

__init__.py Outdated Show resolved Hide resolved

chrisveilleux force-pushed the make-weather-great-again branch from 6d10258 to 9b69f28 Compare April 16, 2021 20:50

forslund reviewed Apr 19, 2021

View reviewed changes

__init__.py Outdated Show resolved Hide resolved

forslund approved these changes Apr 19, 2021

View reviewed changes

__init__.py Show resolved Hide resolved

source/weather.py Outdated Show resolved Hide resolved

source/weather.py Outdated Show resolved Hide resolved

forslund reviewed Apr 24, 2021

View reviewed changes

__init__.py Outdated Show resolved Hide resolved

forslund requested changes Apr 25, 2021

View reviewed changes

forslund reviewed May 6, 2021

View reviewed changes

forslund self-requested a review May 6, 2021 20:59

krisgesling reviewed May 7, 2021

View reviewed changes

chrisveilleux added 12 commits May 12, 2021 12:45

Second pass at refactoring, including reworking the dialog classes, r…

5d38589

…emoving a bunch of condition specific dialog files, code review changes and other minor cleanups.

Bug fixes

4c62179

Add a new GUI screen for a single day of weather.

42924ca

Add a new GUI screen for sunrise and sunset.

a6b73e3

Refactored logic that selected an image or animation to use in the GUI

aad8a27

Added location to the top of weather screens that didn't have it.

730e0c5

renamed "source" directory to "skill"

b39098c

tweaks to GUI screens based on design changes

a8197b5

small bug fix

cdac179

added location data to the forecast screen

05134fb

renamed dialog and vocab files to be lower case and dash delimited.

872cf61

applied code review changes

9ca144a

chrisveilleux force-pushed the make-weather-great-again branch from f84ebe2 to 9ca144a Compare May 12, 2021 17:45

chrisveilleux added 7 commits May 12, 2021 13:21

use the new CardDelegate defined in the mycroft-gui repository to abs…

054c9b2

…tract some of the grid-style card layout commonalities.

Merge remote-tracking branch 'origin/20.08' into make-weather-great-a…

d3e4293

…gain # Conflicts: # ui/DailyDelegateScalable.qml # ui/highlow.qml # ui/weather.qml

resolve merge conflicts

bb5ab99

apply new dialog file naming convention to tests.

04aca83

fixed errant dialog names found in VK testing.

3a19904

fix VK test failures.

47b6baa

Move the rest of the old dialog class to the new ones.

37de4a5

This was linked to issues May 19, 2021

Bump pyowm to latest version #157

Closed

Wrong time reported for sunrise/sunset #137

Closed

Dialog doesn't mirror the forecast on openweathermap #135

Open

krisgesling approved these changes Jun 14, 2021

View reviewed changes

krisgesling merged commit c9244c2 into 20.08 Jun 14, 2021

chrisveilleux deleted the make-weather-great-again branch June 26, 2021 20:25

JarbasAl mentioned this pull request Jun 27, 2021

Use own keys #170

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make weather great again #158

Make weather great again #158

chrisveilleux commented Apr 6, 2021 •

edited

forslund commented Apr 7, 2021

forslund commented Apr 7, 2021

chrisveilleux commented Apr 10, 2021

forslund commented Apr 13, 2021 •

edited

chrisveilleux commented Apr 15, 2021

chrisveilleux commented Apr 16, 2021

forslund commented Apr 19, 2021 •

edited

forslund left a comment

Olzeke51 commented Apr 22, 2021 •

edited

AIIX commented Apr 22, 2021

Olzeke51 commented Apr 22, 2021

chrisveilleux commented Apr 23, 2021

chrisveilleux commented Apr 23, 2021

Olzeke51 commented Apr 23, 2021

forslund commented Apr 23, 2021

forslund left a comment

chrisveilleux commented May 5, 2021

forslund left a comment

krisgesling left a comment

krisgesling May 6, 2021

chrisveilleux May 12, 2021 •

edited

krisgesling May 7, 2021

krisgesling left a comment

Olzeke51 commented Jun 14, 2021

chrisveilleux commented Jun 14, 2021

krisgesling commented Jun 14, 2021

Make weather great again #158

Make weather great again #158

Conversation

chrisveilleux commented Apr 6, 2021 • edited

Description

Type of PR

Testing

Documentation

forslund commented Apr 7, 2021

forslund commented Apr 7, 2021

chrisveilleux commented Apr 10, 2021

forslund commented Apr 13, 2021 • edited

chrisveilleux commented Apr 15, 2021

chrisveilleux commented Apr 16, 2021

forslund commented Apr 19, 2021 • edited

forslund left a comment

Choose a reason for hiding this comment

Olzeke51 commented Apr 22, 2021 • edited

AIIX commented Apr 22, 2021

Olzeke51 commented Apr 22, 2021

chrisveilleux commented Apr 23, 2021

chrisveilleux commented Apr 23, 2021

Olzeke51 commented Apr 23, 2021

forslund commented Apr 23, 2021

forslund left a comment

Choose a reason for hiding this comment

chrisveilleux commented May 5, 2021

forslund left a comment

Choose a reason for hiding this comment

krisgesling left a comment

Choose a reason for hiding this comment

krisgesling May 6, 2021

Choose a reason for hiding this comment

chrisveilleux May 12, 2021 • edited

Choose a reason for hiding this comment

krisgesling May 7, 2021

Choose a reason for hiding this comment

krisgesling left a comment

Choose a reason for hiding this comment

Olzeke51 commented Jun 14, 2021

chrisveilleux commented Jun 14, 2021

krisgesling commented Jun 14, 2021

chrisveilleux commented Apr 6, 2021 •

edited

forslund commented Apr 13, 2021 •

edited

forslund commented Apr 19, 2021 •

edited

Olzeke51 commented Apr 22, 2021 •

edited

chrisveilleux May 12, 2021 •

edited