MyPy flags a type incompatibility in line 436 of Ozone.py. This is in the get_specific_parameter method

[Milind220]

The problem

Here's the warning that's displayed:

Argument of type "Series[Unknown]" cannot be assigned to parameter "x" of type "SupportsFloat | SupportsIndex | str | bytes |

This is the line of code that causes this:

result = float(row[air_param])

Possible explanation

Here, row is a Pandas dataframe, and air_param is a column in it

This warning, if I understand correctly, is basically indicating that MyPy has no guarantee that the value stored in that cell is a number. Due to this, it's not comfortable with us type casting it as a float while assigning it to result.

Possible solution

Better implementation of get_specific_parameter method. This method currently creates an entire dataframe by calling get_city_air, when all it requires is a single number. A more lightweight method of implementing it would be to make an API call, and then get the required parameter directly from the JSON response, and return it from there.

[lahdjirayhan]

Strange, mypy does not complain about it to me (I bet it's something on my end, though). Good catch either way.

---

I don't have anything to say about fixing the mypy warning. However, I have something to say about the possible solution.

I changed the implementation of get_specific_parameter method to this one in this commit because I find it an unnecessary duplication of code that's already in get_city_air. Why write this bit of logic twice?

1. Building a URL.
2. Making request (handled by Ozone via _make_api_request)
3. Checking response status/error code (handled by Ozone via _check_status_code)
4. Checking if the JSON returned is truly OK (I added this check in this commit to handle cities without AQI data. It will also be needed in get_specific_parameter)
5. Parsing the data into more ready-to-use format (handled by Ozone via _parse_data)
6. And finally, take the intended parameter.

I don't think this duplication of logic should be maintained.

If get_city_air and get_specific_parameter should not call one another, I think the better solution is to refactor the implementation so that the bulk of steps above (until before last step) is handled in a private method. get_specific_parameter can take the intended air quality parameter as a float while get_city_air convert all of the parsed results into a dataframe row.

What do you think? @Milind220

[lahdjirayhan]

Also after I think about it, any further fixes to get_city_air will also need to be applied to get_specific_parameter, since basically they're calling the same API URL endpoint and should get roughly the same result. I'm inclined to think refactoring is the best way to go.

[Samxx97]

@lahdjirayhan i agree with line of reasoning of grouping steps from 1 to 5 in a private method for it to be reused to reduce the redundancy in both methods, it makes perfect sense! would you like to hold the beer for this one ?

[Milind220]

@lahdjirayhan Definitely agree with you there - a private method to cover that functionality would be ideal and much cleaner than repeating the code.

@Sam-damn ahahaha I love that line "would you like to hold the beer for this one?" - Great stuff hahaha

[Milind220]

This issue is actually only a symptom of the actual problem that you've pointed out. Let's close this issue and create a new one to address the need for refactoring that section of code.