Ozon3Org / Ozon3

An open-source Python package to easily obtain real-time, historical, or forecasted air quality data for anywhere in the world. Reliable, accurate and simple.
GNU General Public License v3.0
66 stars 23 forks source link

[Breaking Change] params argument lets you enter gibberish and still returns data #140

Closed Milind220 closed 2 years ago

Milind220 commented 2 years ago

Describe the bug With the params argument, If you enter correct parameters, it returns valid data. However, if you enter gibberish as a parameter, it returns data with an empty column for that 'parameter'. For example:

get_city_air(city='London', params=['sldkfjsfljsfsf'])

would return a dataframe with an empty column called 'sldkfjsfljsfsf'.

To Reproduce Steps to reproduce the behavior:

  1. Use any method which allows you to pass in custom params. Eg: get_city_air
  2. Enter in a gibberish parameter
  3. See error

Expected behavior Ozone should warn the user that the parameter passed in is invalid, but go ahead and fetch all valid parameters.

Screenshots

Screenshot 2022-05-03 at 8 07 21 PM

Environment

lahdjirayhan commented 2 years ago

Note to future readers:

This behavior is documented in tests, for example this one.

https://github.com/Milind220/Ozone/blob/ba281f5381cfa0a0c8750715d2486457111ff665/tests/test_get_city_air.py#L68-L74

If this behavior needs to change, then the corresponding tests need to also be changed accordingly.

Milind220 commented 2 years ago

Note to future readers:

This behavior is documented in tests, for example this one.

@lahdjirayhan Ahhh I do remember reading that. Never quite made the connection that it would also mean that random incorrect parameters would still be a part of the response.

Hmm I do think that it's a good idea to warn users about this. I think the most common situation that this would happen in is if the user has a typo in their code (ex 'pm2.6' instead of 'pm2.5'). Might confuse someone.

lahdjirayhan commented 2 years ago

@Milind220 Welp I was thinking it was intended so I accommodated it in tests.

I'll work on this issue.

Hold my beer, I got this

lahdjirayhan commented 2 years ago

Now that I'm working on it I realized something. WAQI never returns specific subset of parameters; it always returns full set of parameters. On every request, Ozone downloads full set of air quality parameters (that are available at the requested AQI station). So, why bother giving the params argument? Why not just return everything to the user and let them subset it themselves?

However, I may be missing a legitimate use case for the params argument to not be removed entirely. Thoughts? @Milind220

EDIT: Please forgive if my language sounds harsh or snarky, I didn't mean to... 😄

Milind220 commented 2 years ago

@lahdjirayhan Heyy! hahaha I think this is totally one of those bits of code that I added in the beginning just to make Ozone seem a little less like a simple wrapper around an API and more like a 'proper package' (just what I thought back in Nov 2021). Here is what I thought when I first wrote Ozone:

Honestly I'm quite onboard with the idea of removing it if it doesn't bother any other code that we've written. It would definitely make Ozone a little more lightweight and less fiddly.

Let's remove it, but somewhere in the documentation add an example where we show users how easy it is to create data subsets on their own if they need to!

Milind220 commented 2 years ago

@lahdjirayhan Let's get rid of this in time for version 3.0 then :)