Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Nov 29, 2020

Conversation

Darumin
Copy link
Contributor

@Darumin Darumin commented Sep 17, 2020

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
Copy link
Owner

csparpa commented Sep 17, 2020

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
Copy link
Contributor Author

Darumin commented Sep 19, 2020

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
Copy link
Owner

csparpa commented Sep 23, 2020

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

@Harmon758
Copy link
Contributor

@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
Copy link
Owner

csparpa commented Nov 26, 2020

@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 csparpa reopened this Nov 26, 2020
@csparpa csparpa merged commit b026b71 into csparpa:develop Nov 29, 2020
@csparpa
Copy link
Owner

csparpa commented Nov 29, 2020

@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

@Darumin Darumin deleted the develop branch June 24, 2021 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants