Add Weather module #31

AyraHikari · 2018-06-23T12:37:56Z

No description provided.

PaulSonOfLars

Looks good, but make sure all comments and variable names are in English, to keep the project consistent :)

PaulSonOfLars · 2018-06-23T12:51:12Z

tg_bot/modules/weather.py

+from telegram.ext import run_async
+
+@run_async
+def cuaca(bot: Bot, update: Update, args: List[str]):


English variables please - keep it consistent

PaulSonOfLars · 2018-06-23T12:52:01Z

tg_bot/modules/weather.py

+        return
+
+    try:
+        bot.sendChatAction(update.effective_chat.id, "typing") # Bot typing before send message


I'm against sending chat actions, as it's unnecessary and causes extra api calls; it just slows down the bot.

PaulSonOfLars · 2018-06-23T12:55:06Z

tg_bot/modules/weather.py

+        update.effective_message.reply_text("Sorry, location not found.")
+    except pyowm.exceptions.api_call_error.APICallError:
+        update.effective_message.reply_text("Write a location to check the weather.")
+    else:


Unnecessary else

PaulSonOfLars · 2018-06-23T12:56:10Z

tg_bot/modules/weather.py

+        cuacanya = observation.get_weather()
+        obs = owm.weather_at_place(location)
+        lokasi = obs.get_location()
+        lokasinya = lokasi.get_name()


Are the above lines guaranteed not to return None?
If yes, chain them up so there's only only necessary variable allocations.
If no, add checks for that.

PaulSonOfLars · 2018-06-23T12:56:34Z

tg_bot/modules/weather.py

+        obs = owm.weather_at_place(location)
+        lokasi = obs.get_location()
+        lokasinya = lokasi.get_name()
+        # statusnya = cuacanya._detailed_status


Nit: remove commented out code

PaulSonOfLars · 2018-06-23T13:00:33Z

tg_bot/modules/weather.py

+
+@run_async
+def cuaca(bot: Bot, update: Update, args: List[str]):
+    location = " ".join(args)


If len(args) == 0: inform user to give a location and then return

PaulSonOfLars · 2018-06-23T13:01:56Z

tg_bot/modules/weather.py

+    except pyowm.exceptions.not_found_error.NotFoundError:
+        update.effective_message.reply_text("Sorry, location not found.")
+    except pyowm.exceptions.api_call_error.APICallError:
+        update.effective_message.reply_text("Write a location to check the weather.")


Does APICallError only occur when no location is specified...?
I believe the length check should fix that, so maybe make this reply more generic?

PaulSonOfLars · 2018-06-23T13:02:23Z

tg_bot/sample_config.py

@@ -34,6 +34,7 @@ class Config(object):
    WORKERS = 8  # Number of subthreads to use. This is the recommended amount - see for yourself what works best!
    BAN_STICKER = 'CAADAgADOwADPPEcAXkko5EB3YGYAg'  # banhammer marie sticker
    ALLOW_EXCL = False  # Allow ! commands as well as /
+    API_OPENWHEATER = None # OpenWeather API


*weather, not wheater

PaulSonOfLars · 2018-06-23T13:02:35Z

tg_bot/__init__.py

@@ -57,6 +57,7 @@
    WORKERS = int(os.environ.get('WORKERS', 8))
    BAN_STICKER = os.environ.get('BAN_STICKER', 'CAADAgADOwADPPEcAXkko5EB3YGYAg')
    ALLOW_EXCL = os.environ.get('ALLOW_EXCL', False)
+    API_WEATHER = os.environ.get('API_OPENWHEATER', None)


PaulSonOfLars · 2018-06-23T13:02:42Z

tg_bot/__init__.py

@@ -98,6 +99,7 @@
    WORKERS = Config.WORKERS
    BAN_STICKER = Config.BAN_STICKER
    ALLOW_EXCL = Config.ALLOW_EXCL
+    API_WEATHER = Config.API_OPENWHEATER


AyraHikari · 2018-06-23T13:42:34Z

I think all fixed in new commit

PaulSonOfLars · 2018-06-23T13:48:03Z

tg_bot/modules/weather.py

+
+from telegram import Message, Chat, Update, Bot
+from telegram.ext import run_async
+


Apply PEP8 import order

standard library

third party

local

PaulSonOfLars · 2018-06-23T13:51:01Z

tg_bot/modules/weather.py

+
+@run_async
+def weather(bot: Bot, update: Update, args: List[str]):
+    if len(args) >= 1:


early returns are preferred over increased indents.

if len(args) == 0: # reply that a location is needed return # do other stuff # no else needed, since returned # less indent = easier to follow ``` This reduces indentation, and makes code easier to read and understand.

PaulSonOfLars · 2018-06-23T13:51:45Z

tg_bot/modules/weather.py

+            return
+
+        try:
+            owm = pyowm.OWM(API_WEATHER, language='en')


given that this is constant, you might want to consider making it a global variable - no need to instantiate it every time the function is called.

PaulSonOfLars · 2018-06-23T13:52:35Z

tg_bot/modules/weather.py

+            observation = owm.weather_at_place(location)
+            theweather = observation.get_weather()
+            getloc = observation.get_location()
+            thelocation = getloc.get_name()


as mentioned in last review, try to chain these if youre 100% sure they wont return None. if there is a chance of None, add checks for it.

If location not found, it will give exception pyowm.exceptions.not_found_error.NotFoundError

PaulSonOfLars · 2018-06-23T13:53:31Z

tg_bot/modules/weather.py

+            theweather = observation.get_weather()
+            getloc = observation.get_location()
+            thelocation = getloc.get_name()
+            temperature = theweather.get_temperature(unit='celsius')['temp']


youre 100% sure that "temp" is in that dict, no chance of a KeyError? if there is, consider using .get()

PaulSonOfLars · 2018-06-23T13:54:28Z

tg_bot/modules/weather.py

+                 status += "☁️ "
+            status += theweather._detailed_status
+
+


PEP8: too much whitespace; double newlines should not be found in a function body. Remove one.

PaulSonOfLars · 2018-06-23T13:54:48Z

tg_bot/modules/weather.py

+
+CUACA_HANDLER = DisableAbleCommandHandler("weather", weather, pass_args=True)
+
+dispatcher.add_handler(CUACA_HANDLER)


Need a newline at EOF

PaulSonOfLars · 2018-06-23T13:55:03Z

tg_bot/modules/weather.py

+
+__mod_name__ = "Weather"
+
+CUACA_HANDLER = DisableAbleCommandHandler("weather", weather, pass_args=True)


cuaca -> weather

- reorder import like - short check if args is none handle - check if location name & temp is none - and changes some code

Added Wheater module

6350507

AyraHikari changed the title ~~Added Wheater module~~ Added Weather module Jun 23, 2018

PaulSonOfLars suggested changes Jun 23, 2018

View reviewed changes

Change comment and some fix for weather module

d749563

PaulSonOfLars suggested changes Jun 23, 2018

View reviewed changes

Update with some changes

85581f3

- reorder import like - short check if args is none handle - check if location name & temp is none - and changes some code

AyraHikari force-pushed the master branch 2 times, most recently from 8a20971 to 85581f3 Compare September 24, 2018 19:04

Remove icon weather

6754d79

AyraHikari changed the title ~~Added Weather module~~ Add Weather module Sep 24, 2018

Added back status message

b1fb1fc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Weather module #31

Add Weather module #31

AyraHikari commented Jun 23, 2018

PaulSonOfLars left a comment

PaulSonOfLars Jun 23, 2018

PaulSonOfLars Jun 23, 2018

PaulSonOfLars Jun 23, 2018

PaulSonOfLars Jun 23, 2018

PaulSonOfLars Jun 23, 2018

PaulSonOfLars Jun 23, 2018

PaulSonOfLars Jun 23, 2018

PaulSonOfLars Jun 23, 2018

PaulSonOfLars Jun 23, 2018

PaulSonOfLars Jun 23, 2018

AyraHikari commented Jun 23, 2018

PaulSonOfLars Jun 23, 2018

PaulSonOfLars Jun 23, 2018 •

edited

Loading

PaulSonOfLars Jun 23, 2018

PaulSonOfLars Jun 23, 2018

AyraHikari Jun 23, 2018

PaulSonOfLars Jun 23, 2018

PaulSonOfLars Jun 23, 2018

PaulSonOfLars Jun 23, 2018

PaulSonOfLars Jun 23, 2018


		from telegram import Message, Chat, Update, Bot
		from telegram.ext import run_async


		CUACA_HANDLER = DisableAbleCommandHandler("weather", weather, pass_args=True)

		dispatcher.add_handler(CUACA_HANDLER)


		__mod_name__ = "Weather"

		CUACA_HANDLER = DisableAbleCommandHandler("weather", weather, pass_args=True)

Add Weather module #31

Are you sure you want to change the base?

Add Weather module #31

Conversation

AyraHikari commented Jun 23, 2018

PaulSonOfLars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AyraHikari commented Jun 23, 2018

Choose a reason for hiding this comment

PaulSonOfLars Jun 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulSonOfLars Jun 23, 2018 •

edited

Loading