csparpa / pyowm

A Python wrapper around the OpenWeatherMap web API
https://pyowm.readthedocs.io
MIT License
789 stars 171 forks source link

Added support for visibility distance/barometric pressure conversion units #338

Closed Darumin closed 3 years ago

Darumin commented 3 years ago

Attempt to fulfill #325 by adding conversions in measurables.py for pressure ("Hg) and visibility (kms/miles), also added unit tests to go with them.

csparpa commented 3 years ago

Hi @Darumin so far it looks nice, thanks!

A couple of improvements:

One - As you've already coded the core conversion logic, I would now advice to expose it through new methods of the Weather class These might be the very similar to the wind() method:

def wind(self, unit='meters_sec'):
  ...

Something like:

def pressure(self, unit='hPa'):
  ...

def visibility_distance(self, unit='m'):
  ...

Please, only make sure that the provided default units are from the International Decimal Metric System (m & Pa)

Two - In order to let the world know and use your excellent work, why don't you also update the technical docs here and the quick code recipes https://github.com/csparpa/pyowm/blob/develop/sphinx/v3/code-recipes.md?

Thanks a lot!

Darumin commented 3 years ago

I added the matching methods to Weather class. I also refactored each use of self.pressure to self.press to avoid collision with the new pressure method.

You'll also see new recipes and utilities usage examples, as requested. Let me know if there's anything else that needs clarity.

csparpa commented 3 years ago

Hello @Darumin thanks for the contribution, it's on the develop branch

Harmon758 commented 3 years ago

@csparpa This doesn't seem to have actually been merged? I can't find the changes in either the develop or master branches, nor in v3.1.1.

Also of note, there's technically a breaking change here, with Weather.pressure being a method instead of an attribute now.

csparpa commented 3 years ago

@Harmon758 I probably have messed the merge up on my local setup :-S I will take a look into that as soon as possible - thanks for pointing the breaking change out

csparpa commented 3 years ago

@Darumin @Harmon758 I've merged this.

As the newly introduced pressure() function was replacing the already existing pressure attribute of Weather objects, I had to rename the function to prevent existing pyowm clients from breaking.

Now the funciton is called barometric_pressure