Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pulled Weather{Warning} out of weather_chance_of_rain.pl, and created we... #357

Merged
merged 3 commits into from
Mar 18, 2014

Conversation

pmatis
Copy link
Contributor

@pmatis pmatis commented Jan 6, 2014

Pulled Weather{Warning} out of weather_chance_of_rain.pl, and created weather_warnings.pl with more features such as announcements and a configurable trigger. Also added the ability to handle multi-line warning messages.

… weather_warnings.pl with more features such as announcements and a configurable trigger. Also added the ability to handle multi-line warning messages.
@hollie
Copy link
Owner

hollie commented Mar 15, 2014

This looks like a valid change to me, but I'm not using this service as I don't live in the US. Anybody who wants to test this further or do we merge this pull request into master as is for the next stable release?

@hollie hollie added this to the Next stable 3.1 milestone Mar 15, 2014
@gac410
Copy link
Contributor

gac410 commented Mar 15, 2014

Hi Lieven,

I don't use the weather service, however the change seems to leave a
bit of extra code and a useless regex capture in
weather_chance_of_rain.pl. I believe that it could be simplified a bit:

if ($line =~/^warning:(.*)/i) {
next;
}

to

next if ($line =~/^warning:/i);

George
.
On 03/15/2014 09:52 AM, Lieven Hollevoet wrote:

This looks like a valid change to me, but I'm not using this service
as I don't live in the US. Anybody who wants to test this further or
do we merge this pull request into master as is for the next stable
release?


Reply to this email directly or view it on GitHub
#357 (comment).

@hollie
Copy link
Owner

hollie commented Mar 15, 2014

Hey George,

Agreed, the extra capture is not required anymore.

…ove Unnecessary Print Log

- Made the regex change proposed by @gac410 re: one line warning regex in chance of rain
- Remove extra 'Return' reference in chance of rain.  This was causing the chance of rain loopcode to exit prematurely.  In my case the was preventing the creating of the "read chance of rain" trigger.
- Removed print log entry of 'WEATHER WARNING' It isnt't necessary as the read command will cause a print log entry if necessary
@krkeegan
Copy link
Collaborator

I tested this out, it appears to work as requested, although I have no weather warnings at the moment to be sure.

I did catch a rather large bug in weather_chance_of_rain. It was issuing a return command outside of any subroutine, and was therefore causing the loopcode to return prematurely.

I made a pull request to the @pmatis repository. Once Steve does his merge, we can merge this one.

Weather: Fix Regex Check for Warnings; Fix Bug in Chance of Rain, Remove Unnecessary Print Log
krkeegan added a commit that referenced this pull request Mar 18, 2014
Pulled Weather{Warning} out of weather_chance_of_rain.pl, and created we...
@krkeegan krkeegan merged commit 836ed61 into hollie:master Mar 18, 2014
@pmatis pmatis deleted the weather_warnings branch September 18, 2016 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants