Full Review #2
Full Review #2
Conversation
Get weather for Lat Lon
| t.Fatalf("GetCoordinates('Castlebar', 'IE') got err %v", err) | ||
| } | ||
| want := meteo.Place{ | ||
| Lng: -9.2988, |
There was a problem hiding this comment.
It's possible we might run into a floating-point imprecision issue comparing these values, isn't it? If Wikipedia provides four decimal places of precision, what about storing the lat and long as int:
Longitude: -92988,
Latitude: 538608You can always insert the decimal point for display.
|
|
||
| var wp wikipediaPlaces | ||
| if err := json.NewDecoder(res.Body).Decode(&wp); err != nil { | ||
| return Place{}, fmt.Errorf("decoding response %w", err) |
There was a problem hiding this comment.
I suggest reading all the body with io.ReadAll and then passing the resulting []byte to json.Unmarshal. In the event of a parsing error, it'll be nice to have the JSON data to display to the user. Using the Decode API, we wouldn't have that, because consuming the reader destroys its contents.
| // GetWeather returns current weather for given | ||
| // place and country using default client for the Norwegian | ||
| // meteorological Institute. | ||
| func GetWeather(place, country string) (Weather, error) { |
There was a problem hiding this comment.
Different weather providers have different ways to specify the location, so perhaps we need to think more generally when defining our GetWeather function, because it'll become frozen as an interface.
| func GetWeather(place, country string) (Weather, error) { | |
| func GetWeather(location string) (Weather, error) { |
Each provider (for example Norway) can interpret that location string according to its own needs.
There was a problem hiding this comment.
(Alternatively, you could define a struct Location, with multiple ways of specifying a location. To save paperwork, you could then provide a few different ways to specify a location, by city-country, by lat/long, What3Words, and so on.)
| if err != nil { | ||
| return Weather{}, err | ||
| } | ||
| c, err := NewNorwayClient(resolver) |
There was a problem hiding this comment.
I think the geo-resolver stuff is independent enough to be moved out into its own package, isn't it? I mean, I can see myself using it for something not related to weather, so I wouldn't want to import the whole meteo package just to get the lat/long functionality.
There was a problem hiding this comment.
implemented. For now in the same repo, but I am thinking in the future to move it to a separate repo and add more functionality to the library to cover all functionality of the current GeoNames web services (https://www.geonames.org/export/ws-overview.html)
A full review of the entire repository. When this PR is complete, be sure to manually merge its head branch into the main branch for this repository.