[Feature] Units conversion using parameters #12
Conversation
rarycoringa
force-pushed
the
feature/units-params
branch
from
03e4efe to
b055cc2
Compare
…with the new modeling
| unit = StringType(required=True) | ||
|
|
||
|
|
||
| class Temperature(Model): |
There was a problem hiding this comment.
Overall, this is quite an improvement from what we've had so far. Congrats.
| temperature = self.context.get('temperature', 'kelvin') | ||
|
|
||
| if temperature.upper() == self.TemperatureMetrics.CELSIUS: | ||
| value = round(self.value - 273.0, 2) # Converts Kelvin temperature to Celsius |
There was a problem hiding this comment.
Maybe instead of rounding to 2 dpp every time you could use a custom type inheriting from FloatType which specifies that centrally.
| temperature = self.context.get('temperature', 'kelvin') | ||
|
|
||
| if temperature.upper() == self.TemperatureMetrics.CELSIUS: | ||
| value = round(self.value - 273.0, 2) # Converts Kelvin temperature to Celsius |
There was a problem hiding this comment.
As a side note, rounding floats is not very reliable. This reference is quite outdated but still relevant.
| return unit.lower() | ||
|
|
||
|
|
||
| class Distance(Model): |
There was a problem hiding this comment.
Maybe Distance and Temperature could inherit from an generic abstract "Number with unit" data type, leaving only the metrics, the default one and the transformation functions between each pair (or to and from default) to be defined in each of these.
That would add quite a bit of complexity though and would only be justifiable for production code if there were to exist more classes like these.
| def __new__(mcs, name, bases, attrs): | ||
| attrs['choices'] = [v for k, v in attrs.items( | ||
| ) if not k.startswith('_') and k.isupper()] | ||
| return TypeMeta.__new__(mcs, name, bases, attrs) |
There was a problem hiding this comment.
I know I've sent this to you, but do you understand what it does?
dluiscosta
left a comment
dluiscosta
left a comment
There was a problem hiding this comment.
Although I'm submitting this review, working on this repo should be de-prioritized for now,
| 'min_temperature': Temperature({ | ||
| 'value': float(response['main']['temp_min']) | ||
| }) if response['main']['temp_min'] != response['main']['temp'] else None, | ||
| 'wind': Wind({ |
There was a problem hiding this comment.
This one should follow the changes of the other fields as well.
|
|
||
| @serializable(type=StringType, serialized_name='id') | ||
| def id(self): | ||
| return unidecode(f'{self.city.name.upper()}') |
There was a problem hiding this comment.
At PassFort's team we use UUID. Please, take a look.
This PR add a new unit conversion feature.
New models available
Now, two models of metrics are available:
This models manage the default value requested with the OpenWeatherMap API and return for the user the value converted to the desired unit.
This management is done by the context attribute.
Usage the feature
GET
http://localhost:5000/weather/{city_name}get a city weatherParameters
temperaturekelvin,celsiusandfahrenheit. If not informed, the default value will bekelvin.distancemeters.Responses
This is the result for
http://localhost:5000/weather/londonwithtemperature=fahrenheitanddistance=miles:{ "city": { "name": "London", "country": "GB", "coordinates": [51.5085, -0.1257] }, "description": "Clouds", "long_description": "Broken Clouds", "temperature": { "value": 53.92, "unit": "fahrenheit" }, "feels_like": { "value": 51.33, "unit": "fahrenheit" }, "max_temperature": { "value": 56.26, "unit": "fahrenheit" }, "min_temperature": { "value": 53.28, "unit": "fahrenheit" }, "wind": { "speed": { "value": 10.8, "unit": "meters per second" }, "degree": { "value": 270.0, "unit": "degrees" } }, "visibility": { "value": 6.21, "unit": "miles" }, "id": "LONDON" }