Conversation
|
Can you update your branch please to the latest master? I've fixed the lint warnings in master today. |
|
Please also add a section to README.md on how to configure it correctly. |
There was a problem hiding this comment.
Pull request overview
Adds a new yr.no backend to the existing wego backend plugin system, enabling forecasts to be fetched from the Norwegian Meteorological Institute’s Locationforecast API (api.met.no).
Changes:
- Introduces a new backend implementation
yr.nowith flag-based configuration (User-Agent, debug). - Implements fetching + JSON unmarshalling of the Locationforecast “compact” response.
- Adds parsing logic to convert yr.no timeseries entries into
iface.Data(Current+ multi-day forecast).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| req, err := http.NewRequest("GET", url, nil) | ||
| if err != nil { | ||
| log.Fatalln(err) | ||
| } |
There was a problem hiding this comment.
fetch() calls log.Fatalln(err) if http.NewRequest fails, which terminates the whole program even though the method already returns an error. Prefer returning a wrapped error here so callers can handle/report it consistently (like the other error paths in this function).
| if err != nil { | ||
| return nil, fmt.Errorf("unable to read response body (%s): %v", url, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
fetch() does not check res.StatusCode before reading/unmarshalling the body. If yr.no returns a non-200 (e.g., 403 for missing/invalid User-Agent or 400 for invalid coords), the code will try to unmarshal an error/HTML response and later logic may panic on empty data. Add an explicit non-2xx status check and return a clear error including status code/body snippet.
| if res.StatusCode < http.StatusOK || res.StatusCode >= http.StatusMultipleChoices { | |
| bodySnippet := strings.TrimSpace(string(body)) | |
| if len(bodySnippet) > 512 { | |
| bodySnippet = bodySnippet[:512] + "..." | |
| } | |
| return nil, fmt.Errorf("unexpected response status for %s: %s; body: %s", url, res.Status, bodySnippet) | |
| } |
| if matched, err := regexp.MatchString(`^-?[0-9]*(\.[0-9]+)?,-?[0-9]*(\.[0-9]+)?$`, location); matched && err == nil { | ||
| s := strings.Split(location, ",") | ||
| loc = fmt.Sprintf("lat=%s&lon=%s", s[0], s[1]) | ||
| ret.Location = location | ||
| } | ||
|
|
||
| resp, err := c.fetch(fmt.Sprintf(yrNoURI, loc)) | ||
| if err != nil { |
There was a problem hiding this comment.
If location is not a lat,lon pair, loc remains empty but the code still calls the API with an empty query string. This should fail fast with a helpful message (similar to smhi), e.g., log.Fatalf explaining that this backend only supports latitude,longitude input.
| resp, err := c.fetch(fmt.Sprintf(yrNoURI, loc)) | ||
| if err != nil { | ||
| log.Fatalf("Failed to fetch weather data: %v\n", err) | ||
| } | ||
| ret.Current, err = c.parseCond(resp.Properties.Timeseries[0]) | ||
|
|
There was a problem hiding this comment.
resp.Properties.Timeseries[0] is accessed without checking that Timeseries is non-empty. In error scenarios (or unexpected API changes) this can panic with an index-out-of-range. Guard with a length check and return/exit with an explicit "no timeseries in response" error.
| day.Slots = append(day.Slots, slot) | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
parseDaily() never appends the final day after the loop ends, so if the loop completes naturally (e.g., user requests all available days) the last day of forecast is dropped. Append *day after the loop when day != nil and len(forecast) < numdays (or when numdays exceeds available data).
| } | |
| } | |
| if day != nil && len(forecast) < numdays { | |
| forecast = append(forecast, *day) | |
| } |
| func (c *yrNoConfig) parseCond(entry timeSeriesEntry) (iface.Cond, error) { | ||
| var ret iface.Cond | ||
| // descriptions from https://github.com/metno/weathericons/blob/main/weather/legend.csv | ||
| descriptionMap := map[string]string{ | ||
| "clearsky": "Clear sky", | ||
| "fair": "Fair", | ||
| "partlycloudy": "Partly cloudy", |
There was a problem hiding this comment.
parseCond() rebuilds large descriptionMap/codemap maps on every call, which is expensive given this runs once per timeseries entry. Move these maps to package-level var/const (or initialize once) to avoid repeated allocations and improve throughput.
| } | ||
| precipM := entry.Data.NextOneHour.Details.Precipitation / 1000. | ||
| ret.PrecipM = &precipM | ||
| ret.Time, _ = time.Parse(time.RFC3339, entry.Dt) |
There was a problem hiding this comment.
parseCond() ignores the time.Parse error (ret.Time, _ = ...) and still returns nil error, which can silently produce a zero timestamp and break day grouping/sorting. Handle the parse error and return it to the caller so parseDaily()/Fetch() can log/skip appropriately.
| ret.Time, _ = time.Parse(time.RFC3339, entry.Dt) | |
| parsedTime, err := time.Parse(time.RFC3339, entry.Dt) | |
| if err != nil { | |
| return ret, err | |
| } | |
| ret.Time = parsedTime |
Motivation and Context
https://yr.no is a norwegian website ("A collaboration between NRK and The Norwegian Meteorological Institute") offering weather data with (in my opinion) very usage-friendly API conditions.
Description
I added a backend for the yr.no locationforecast api.
This is the first time I wrote golang code, so please review and let me know where I messed up, I am happy to fix it.
Steps for Testing
./wego -yrno-user-agent "wego CLI, your@email.local" -b yr.noScreenshots