raman325 / pytomorrowio

Async Python client for Tomorrow.io API
MIT License
2 stars 3 forks source link

Make rate limit headers case insensitive #10

Closed lymanepp closed 2 years ago

codecov-commenter commented 2 years ago

Codecov Report

Merging #10 (a070c29) into master (467f968) will increase coverage by 13.80%. The diff coverage is 94.11%.

@@             Coverage Diff             @@
##           master      #10       +/-   ##
===========================================
+ Coverage   71.42%   85.23%   +13.80%     
===========================================
  Files           7        7               
  Lines         315      325       +10     
===========================================
+ Hits          225      277       +52     
+ Misses         90       48       -42     
Impacted Files Coverage Δ
pytomorrowio/pytomorrowio.py 83.52% <94.11%> (+27.27%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 467f968...a070c29. Read the comment docs.

lymanepp commented 2 years ago

@raman325 I have some thoughts after writing tests for realtime_and_all_forecasts.

My suggestion is to remove this method as everything is either implemented elsewhere or has problems that can't be resolved.

lymanepp commented 2 years ago

HI @raman325, I merged your changes to add num_api_requests to the API so that I could verify that my refactoring mocking was working right. I didn't merge anything related to MAX_FIELDS.

raman325 commented 2 years ago

@raman325 I have some thoughts after writing tests for realtime_and_all_forecasts.

  • The method is doing a lot of things and is subtly complex. That makes it harder to maintain, test and use.
  • Most of the things that it does can be done by the realtime, forecast_nowcast, forecast_hourly, and forecast_daily methods.
  • The only exception is the code branch when hourly_fields and daily_fields are None. In that case, a common set of fields are requested for nowcast, hourly and daily forecasts. But the Min, Max, and Avg fields that make sense for daily, don't make sense for the other forecasts. And other fields are not available for daily forecasts (e.g., precipitationType, particulateMatter25, treeIndex, etc).

My suggestion is to remove this method as everything is either implemented elsewhere or has problems that can't be resolved.

I don't know that I want to reuse forecast_nowcast, forecast_hourly, and forecast_daily because it will increase the number of API calls we make. But we can clean up _forecast and perhaps call that directly. I do agree that we should call realtime within this umbrella method and get rid of the custom logic.