Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion .github/workflows/python-linting.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,11 @@ jobs:
# Run our linting
- name: Lint code
run: |
./bin/task lint
./bin/task lint

# Testing
- name: Test code
env:
TNT_EX1_OPENWEATHERMAP_API_KEY: ${{ secrets.TNT_EX1_OPENWEATHERMAP_API_KEY}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not supposed to happen 🤯 you ought to mock the environment variable away. Unit test means to test a standalone unit 😘

run:
./bin/task test
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a newline at the end of the file since some tools don't execute the last line if it ends with EOF (end of file) instead of a newline \n.

11 changes: 10 additions & 1 deletion Taskfile.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,13 @@ tasks:
run:
desc: Run the Weather Data Fetcher
cmds:
- poetry run python weather_data_fetcher.py
- poetry run python weather_data_fetcher.py

test:
desc: Runs tests on the code
cmds:
- >
poetry run pytest
-s
Copy link
Contributor

Choose a reason for hiding this comment

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

Pleas write out command line options if you can. I have no fricking clue what -s is ❓

--cov=.
--cov-report=html
Copy link
Contributor

Choose a reason for hiding this comment

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

Another newline 👀

139 changes: 138 additions & 1 deletion poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ python-dotenv = "^0.21.0"
pylint = "^2.15.2"
black = "^22.8.0"
flake8 = "^5.0.4"
pytest = "^7.1.3"
pytest-cov = "^4.0.0"

[build-system]
requires = ["poetry-core>=1.0.0"]
Expand Down
61 changes: 61 additions & 0 deletions test_weather_data_fetcher.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import os
import dotenv
from unittest import mock

from weather_data_fetcher import fetch_location_coords, fetch_weather_data, get_api_key


class TestWeatherDataFetcher:
def test_fetch_coords(self):
coords_berlin = fetch_location_coords("Berlin")
Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot to mock the real API call underneath. Please do that otherwise I cannot run this test without internet.

coords_difference = [sum(x) for x in zip(coords_berlin, (-52.520008, -13.404954))]
assert sum(list(coords_difference)) < 0.01
Comment on lines +10 to +12
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting way to test this 😅 is there maybe an easier and more transparent way to possible test this? Like checking if something equals something without summing up and other tricks?


@mock.patch("weather_data_fetcher.fetch_new_data", autospec=True)
def test_fetch_data(self, request_mock):
request_mock.return_value = {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can make your life easier here and just put into the json payload what you extract in the end. This makes everything much much shorter.

"cod": "200",
"message": 0,
"cnt": 40,
"list": [
{
"dt": 1665824400,
"main": {
"temp": 12.95,
"feels_like": 12.76,
"temp_min": 12.95,
"temp_max": 13.93,
"pressure": 1000,
"sea_level": 1000,
"grnd_level": 1004,
"humidity": 94,
"temp_kf": -0.98,
},
"weather": [
{"id": 803, "main": "Clouds", "description": "broken clouds", "icon": "04d"}
],
"clouds": {"all": 81},
"wind": {"speed": 1.55, "deg": 215, "gust": 3.1},
"visibility": 10000,
"pop": 0,
"sys": {"pod": "d"},
"dt_txt": "2022-10-15 09:00:00",
},
],
"city": {
"id": 7576815,
"name": "Alt-Kölln",
"coord": {"lat": 52.517, "lon": 13.3889},
"country": "DE",
"population": 2000,
"timezone": 7200,
"sunrise": 1665811879,
"sunset": 1665850372,
},
}
if "TNT_EX1_OPENWEATHERMAP_API_KEY" not in os.environ:
dotenv_file = dotenv.find_dotenv()
dotenv.load_dotenv(dotenv_file)
Comment on lines +56 to +58
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this and mock the environment variable getter.


fetch_weather_data(os.environ["TNT_EX1_OPENWEATHERMAP_API_KEY"], (52.520008, 13.404954))
Copy link
Contributor

Choose a reason for hiding this comment

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

It is nice how you call the function here, but you only check that it does not error out basically. If you really want to know what was done inside, you need to mock the print or stdout and verify how the output message would look like with an assert.

assert request_mock.called
35 changes: 21 additions & 14 deletions weather_data_fetcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from geopy.geocoders import Nominatim


def fetch_weather_data(api_key: str, location_coords: Tuple[int, int]) -> None:
def fetch_weather_data(api_key: str, location_coords: Tuple[float, float]) -> None:
"""
Fetches weather data for 25 timepoints from the url if no recent data is available
Stores the data in the folder where it is executed for future use.
Expand Down Expand Up @@ -47,19 +47,7 @@ def fetch_weather_data(api_key: str, location_coords: Tuple[int, int]) -> None:
fetch_data_from_url = True

if not os.path.exists(file_path_json) or fetch_data_from_url:
print("Getting new data from URL", file=sys.stderr)
response = requests.get(
"https://api.openweathermap.org/data/2.5/forecast",
params={
"lat": location_coords[0],
"lon": location_coords[1],
"dt": 25,
"units": "metric",
"appid": api_key,
},
timeout=15,
)
data = response.json()
data = fetch_new_data(api_key, location_coords)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is so beautiful and smart 💡 I'm proud of you 😘

with open(file_path_json, "w", encoding="utf-8") as file:
json.dump(data, file, ensure_ascii=False, indent=4)
else:
Expand All @@ -81,6 +69,25 @@ def fetch_weather_data(api_key: str, location_coords: Tuple[int, int]) -> None:
print(output)


def fetch_new_data(api_key: str, location_coords: Tuple[float, float]) -> json:
Copy link
Contributor

Choose a reason for hiding this comment

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

You have two choices:

  • Make the function an internal function by starting with an underscore _
  • Write a more complete docstring describing the function including the parameters and return value

"""
Uses the request module to fetch new data
"""
print("Getting new data from URL", file=sys.stderr)
response = requests.get(
"https://api.openweathermap.org/data/2.5/forecast",
params={
"lat": location_coords[0],
"lon": location_coords[1],
"dt": 25,
"units": "metric",
"appid": api_key,
},
timeout=15,
)
return response.json()


def fetch_location_coords(city: str) -> Tuple[int, int]:
"""
Gets the (lat, long) coordinates of a city name. If the city cannot be found
Expand Down