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.

Sign up for GitHub

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

Feature: Added celsius support for weather #19

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Feature: Added celsius support for weather #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4abcbd2
Feature: Added celsius support for weather
Aug 1, 2016
477e33b
Fix: Added documentation
Aug 1, 2016
a50d279
Fix: Fixed documentation spacing
Aug 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion DOCS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,12 +172,13 @@ This posts weather information of a given location.

__Usage__

`@weather <zipcode | location name>`
`@weather <zipcode | location name> > <degree type>`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format this doc to adopt the flag notation.


__Arguments__

* `zipcode`: A five digit US zipcode.
* `location name`: A letter and space only location name, or city name and state separated by a comma and a space.
* `degree type`: A degree type to display the weather, the parameter can be 'C' for Celsius or 'F' for Fahrenheit. The default type is Fahrenheit.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can also support lower case for lazy people who don't want to press shift.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was thinking about that, I'm going to do some toUpperCase stuff and it should be good to go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like flags are overkill? This is messenger, not bash.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjchee I might stick with the original implementation with the updated regex so hopefully the nasty loop will disappear.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjchee The reason behind the flag is to sanitize the input. Right now we are polling the last character of the address, which is pretty sketch. Also, if we designate the flag syntax, then we can reuse old code after substringing the flag out, so we don't need to do deep regression testing.

Copy link
Contributor

@rjchee rjchee Aug 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@renxinhe Why can't we just poll for a space and a C or an F? I doubt there are locations that match that, and if there were, users can just tack on the degree type they want to use. Then the old code can still be reused, and we don't have to introduce bash to non-programmer users.


__Example__

Expand Down Expand Up @@ -205,3 +206,14 @@ Feels like 73°. Humidity 94%.
08-02 | 71°/76° :umbrella: 80%
08-03 | 72°/78° :umbrella: 60%

>`@weather 78701 C`

>37°C
Austin, TX
Sunny :sunny:
Feels like 37°. Humidity 35%.
07-31 | 24°/35°
08-01 | 24°/36° :umbrella: 0%
08-02 | 26°/37° :umbrella: 0%
08-03 | 26°/38° :umbrella: 0%
08-04 | 26°/37° :umbrella: 30%
2 changes: 1 addition & 1 deletion public/javascript/bot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ function messageHandler(event) {
handlerFunctions['getPokemon'](userAPI, event.threadID, event.senderID, message);
} else if ((/^@stock .+$/).test(message)) {
handlerFunctions['getStock'](userAPI, event.threadID, message);
} else if ((/^@weather ([0-9]{5}|([a-zA-Z ]+(, )?[a-zA-Z ]+))$/).test(message)) {
} else if ((/^@weather ([0-9]{5} [CF]|([a-zA-Z ]+(, )?[a-zA-Z ]+))$/).test(message)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the degree type into flags --unit <C/F> and -u <C/F>? And modify the regex accordingly to sanitize the input.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure how to do that? Any tutorials?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or you want a format like @weather 78701 --unit C

Copy link
Owner

@renxinhe renxinhe Aug 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah like that. @weather 78701 --unit C, or @weather 78701 -u F, or lower cases.

If we use flags, we are guaranteed that the characters after the - are not part of the address, so we can simply substring the flag out, and use the @weather 78701 part of the body the old way.

handlerFunctions['getWeather'](userAPI, event.threadID, message);
} else if ((/^@meme$/).test(message)) {
handlerFunctions['rickroll'](userAPI, event.threadID);
Expand Down
23 changes: 18 additions & 5 deletions public/javascript/getWeather.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,22 @@
var weather = require("weather-js");

module.exports = function getWeather(api, threadID, body) {
const locale = body.substring('@weather '.length);
console.log('Fetching weather for ' + locale + '...');
weather.find({ search: locale, degreeType: 'F' }, function(err, result) {
var locale = '';
var currentDType = 'F';
const infoArray = body.split(' ');
for (var i = 1; i < infoArray.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop seems like a lot of work for just basically checking if the last thing is a C or an F.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I was thinking about extracting the last part first and process it afterwards.

if (i == 1) {
locale += infoArray[i];
} else {
if (i != infoArray.length - 1 || (i == infoArray.length - 1 && infoArray[infoArray.length - 1] != 'F' && infoArray[infoArray.length - 1] != 'C')) {
locale += ' ' + infoArray[i];
} else {
currentDType = infoArray[infoArray.length - 1];
}
}
}
console.log('Fetching weather for ' + locale + ' with degreeType ' + currentDType + '...');
weather.find({ search: locale, degreeType: currentDType }, function(err, result) {
if (err) {
api.sendMessage(err, threadID);
console.error(err);
Expand Down Expand Up @@ -65,9 +78,9 @@ module.exports = function getWeather(api, threadID, body) {
} else {
emoji = '';
}

var displayType = currentDType === 'F' ? '\xB0F\n' : '\xB0C\n';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var displayType = '\xB0' + currentDType + '\n';
is cleaner lol.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be a const as well right? Considering we aren't changing the display degree type afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably

let message =
data.current.temperature + '\xB0F\n' + data.location.name + '\n' + data.current.skytext + emoji + '\n' +
data.current.temperature + displayType + data.location.name + '\n' + data.current.skytext + emoji + '\n' +
'Feels like ' + data.current.feelslike + '\xB0. Humidity ' + data.current.humidity + '%.\n';

// Concatenate forecast to message
Expand Down