Add solarprognose.de - Neuer PR

[thlink68]

Änderungsvorschläge @tolot27 eingearbeitet.

Link zu PR #19364

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• Simplify the nested conditional logic in the URI template (item → id/uniquetoken) to reduce complexity and improve readability.
• Ensure that any special characters in the access-token or uniquetoken are properly URL-encoded in the rendered URI to avoid malformed requests.
• Consider extracting the repeated strftime logic in the jq block into a shared helper or snippet to improve maintainability if similar patterns exist elsewhere.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- Simplify the nested conditional logic in the URI template (item → id/uniquetoken) to reduce complexity and improve readability.
- Ensure that any special characters in the access-token or uniquetoken are properly URL-encoded in the rendered URI to avoid malformed requests.
- Consider extracting the repeated strftime logic in the jq block into a shared helper or snippet to improve maintainability if similar patterns exist elsewhere.

## Individual Comments

### Comment 1
<location> `templates/definition/tariff/solarprognose.yaml:52` </location>
<code_context>
+    source: http
+    uri: https://www.solarprognose.de/web/solarprediction/api/v1?_format=json&access-token={{ .token }}{{ if .item }}&item={{ .item }}{{ if .id }}&id={{ .id }}{{ else }}{{ if .uniquetoken }}&token={{ unquote .uniquetoken }}{{ end }}{{ end }}{{ end }}&algorithm={{ .algorithm }}&type=hourly
+    jq: |
+      [ .data | to_entries.[] | {
+        "start": (.key | tonumber | strftime("%FT%TZ") ),
+        "end": (.key | tonumber + 3600 | strftime("%FT%TZ")  ) ,
+        "value": (.value.[0]*1000)
+      } ] | tostring
+  interval: {{ .interval }}
</code_context>

<issue_to_address>
Multiplying '.value.[0]' by 1000 assumes the unit is always kW; consider clarifying or validating units.

If the API changes units or the value isn't always in kW, this conversion could produce incorrect results. Please document the unit assumption or add a validation step to confirm units before conversion.

Suggested implementation:

```
    # Assumes .value.[0] is in kW; multiplies by 1000 to convert to W.
    # If the API response includes a unit field, validate it before conversion.
    jq: |
      [ .data | to_entries.[] | {
        "start": (.key | tonumber | strftime("%FT%TZ") ),
        "end": (.key | tonumber + 3600 | strftime("%FT%TZ")  ) ,
        "value": (
          if (.value_unit? // "kW") == "kW" then
            (.value.[0] * 1000)
          else
            error("Unexpected unit for value: \(.value_unit)")
          end
        )
      } ] | tostring

```

- If the API does not provide a unit field (e.g., `.value_unit`), you may need to adjust the jq filter to match the actual field name or remove the validation and rely solely on the documented assumption.
- If the unit field is nested differently, update `.value_unit?` to the correct path.
- If you want to handle other units (e.g., W, MW), extend the jq filter accordingly.
</issue_to_address>

### Comment 2
<location> `templates/definition/tariff/solarprognose.yaml:53` </location>
<code_context>
+    uri: https://www.solarprognose.de/web/solarprediction/api/v1?_format=json&access-token={{ .token }}{{ if .item }}&item={{ .item }}{{ if .id }}&id={{ .id }}{{ else }}{{ if .uniquetoken }}&token={{ unquote .uniquetoken }}{{ end }}{{ end }}{{ end }}&algorithm={{ .algorithm }}&type=hourly
+    jq: |
+      [ .data | to_entries.[] | {
+        "start": (.key | tonumber | strftime("%FT%TZ") ),
+        "end": (.key | tonumber + 3600 | strftime("%FT%TZ")  ) ,
+        "value": (.value.[0]*1000)
+      } ] | tostring
</code_context>

<issue_to_address>
Assumes '.key' is a Unix timestamp in seconds; consider handling non-standard formats.

If '.key' is not a Unix timestamp in seconds, this logic will fail. Please add validation or a fallback to handle unexpected formats.

Suggested implementation:

```
    jq: |
      [ .data | to_entries.[] | 
        select(
          (.key | test("^[0-9]+$")) and
          (.key | tonumber >= 0)
        ) 
        | {
          "start": (.key | tonumber | strftime("%FT%TZ")),
          "end": (.key | tonumber + 3600 | strftime("%FT%TZ")),
          "value": (.value.[0]*1000)
        }
        ,
        # Fallback: handle non-standard formats (try to parse as RFC3339, else null)
        (.data | to_entries.[] | 
          select(
            (.key | test("^[0-9]+$") | not) or
            (.key | tonumber < 0)
          )
          | {
            "start": (.key | strptime("%Y-%m-%dT%H:%M:%S%z")? // null | strftime("%FT%TZ") ),
            "end": null,
            "value": (.value.[0]*1000)
          }
        )
      ] | tostring

```

- If you want to handle other date formats, adjust the `strptime` pattern in the fallback block.
- If you want to log or collect invalid entries, you may need to add a separate array or field for them.
- If you want to skip non-standard keys entirely, remove the fallback block.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion: Multiplying '.value.[0]' by 1000 assumes the unit is always kW; consider clarifying or validating units.

If the API changes units or the value isn't always in kW, this conversion could produce incorrect results. Please document the unit assumption or add a validation step to confirm units before conversion.

Suggested implementation:

    # Assumes .value.[0] is in kW; multiplies by 1000 to convert to W.
    # If the API response includes a unit field, validate it before conversion.
    jq: |
      [ .data | to_entries.[] | {
        "start": (.key | tonumber | strftime("%FT%TZ") ),
        "end": (.key | tonumber + 3600 | strftime("%FT%TZ")  ) ,
        "value": (
          if (.value_unit? // "kW") == "kW" then
            (.value.[0] * 1000)
          else
            error("Unexpected unit for value: \(.value_unit)")
          end
        )
      } ] | tostring

• If the API does not provide a unit field (e.g., .value_unit), you may need to adjust the jq filter to match the actual field name or remove the validation and rely solely on the documented assumption.
• If the unit field is nested differently, update .value_unit? to the correct path.
• If you want to handle other units (e.g., W, MW), extend the jq filter accordingly.

[tolot27]

I've confirmed that the returned value unit is always kW.

[sourcery-ai]

Thanks for confirming that the API always returns values in kW. In that case, multiplying by 1000 to convert to W is appropriate, and you can document this assumption in the code comments for future reference. If the API ever changes, revisiting this conversion would be a good idea, but for now your implementation is correct.

Let me know if you need help with anything else!

[andig]

I've confirmed that the returned value unit is always kW.

Then remove the complexity?

[sourcery-ai]

Absolutely, since the unit is always kW according to your confirmation, you can keep the conversion simple and remove the extra validation logic. Just ensure there's a clear comment in the code noting that the API returns kW, and the multiplication by 1000 is to convert to W. This keeps the code clean and maintainable.

If the API ever changes, you can revisit this, but for now, simplicity is best!

[sourcery-ai]

suggestion: Assumes '.key' is a Unix timestamp in seconds; consider handling non-standard formats.

If '.key' is not a Unix timestamp in seconds, this logic will fail. Please add validation or a fallback to handle unexpected formats.

Suggested implementation:

    jq: |
      [ .data | to_entries.[] | 
        select(
          (.key | test("^[0-9]+$")) and
          (.key | tonumber >= 0)
        ) 
        | {
          "start": (.key | tonumber | strftime("%FT%TZ")),
          "end": (.key | tonumber + 3600 | strftime("%FT%TZ")),
          "value": (.value.[0]*1000)
        }
        ,
        # Fallback: handle non-standard formats (try to parse as RFC3339, else null)
        (.data | to_entries.[] | 
          select(
            (.key | test("^[0-9]+$") | not) or
            (.key | tonumber < 0)
          )
          | {
            "start": (.key | strptime("%Y-%m-%dT%H:%M:%S%z")? // null | strftime("%FT%TZ") ),
            "end": null,
            "value": (.value.[0]*1000)
          }
        )
      ] | tostring

• If you want to handle other date formats, adjust the strptime pattern in the fallback block.
• If you want to log or collect invalid entries, you may need to add a separate array or field for them.
• If you want to skip non-standard keys entirely, remove the fallback block.

[tolot27]

Okay, nun hast du einen neuen Pull Request erzeugt. Bitte verlinke den alten damit und wähle einen passenden Titel.

[tolot27]

Sieht nun gut aus.
In einem separaten PR kann man dann noch die Implementierung von preferredNextApiRequestAt vornehmen.

[tolot27]

Ich habe mir das doch nochmal mit der nested conditional logic im URI template angeschaut und durch pre-computed parameters ersetzt, damit es übersichtlicher und wartbarer wird. Außerdem habe ich die Reihenfolge der Parameter angepasst.

render: |
  {{- define "buildItemParams" -}}
    {{- if .item -}}
      {{- printf "&item=%s" .item -}}
      {{- if .id -}}
        {{- printf "&id=%s" .id -}}
      {{- else if .uniquetoken -}}
        {{- printf "&token=%s" (unquote .uniquetoken) -}}
      {{- end -}}
    {{- end -}}
  {{- end -}}
  type: custom
  tariff: solar
  forecast:
    source: http
    uri: https://www.solarprognose.de/web/solarprediction/api/v1?_format=json&type=hourly&algorithm={{ .algorithm }}&access-token={{ .token }}{{ template "buildItemParams" . }}
    jq: |
      [ .data | to_entries.[] | {
        "start": (.key | tonumber | strftime("%FT%TZ") ),
        "end": (.key | tonumber + 3600 | strftime("%FT%TZ")  ) ,
        "value": (.value.[0]*1000) # returned value unit is always kW
      } ] | tostring
  interval: {{ .interval }}

[andig]

buildItemParams

Finden wir dafür noch einen nachvollziehbaren Namen? Sonst würde ich- da ja nciht wiederverwendet- sogar eher die alte Version stehen lassen.

[tolot27]

buildItemParams

Finden wir dafür noch einen nachvollziehbaren Namen? Sonst würde ich- da ja nciht wiederverwendet- sogar eher die alte Version stehen lassen.

Wir könnten es noch optItemParams oder optionalItemParams nennen. item ist ja optional. Falls es benötigt wird, muss man entweder id oder token als weiteren Parameter der URL mitgeben. Das Template buildItemParams zu nennen, fand ich daher zutreffend. Es kann auch gern anders heißen. Ich bin auch mit der früheren Variante einverstanden, wenn die nested item logic als letzter Parameter übergeben wird. Das verschachtelte Konstrukt ist wegen den vielen geschweiften Klammern nur schwer lesbar.

Damit die ursprüngliche Version besser verständlich ist, schlage ich folgende Variante mit Kommentar vor, wo zudem nicht mehr geprüft wird, ob uniquetoken einen Wert hat. Falls nicht, ist es ohnehin egal, wie die URL aussieht, da immer ein Fehler von der API zurückkommen würde. Ich glaub das ist die sinnvollste Variante.

...
render: |
  type: custom
  tariff: solar
  forecast:
    source: http
    # Base URL with required params (format, type, algorithm, access token), then optional item-based parameters
    uri: https://www.solarprognose.de/web/solarprediction/api/v1?_format=json&type=hourly&algorithm={{ .algorithm }}&access-token={{ .token }}{{ if .item }}&item={{ .item }}{{ if .id }}&id={{ .id }}{{ else }}&token={{ unquote .uniquetoken }}{{ end }}{{ end }}
...

[andig]

Perfekt!

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• The uniquetoken parameter name overlaps with token and may be confusing—consider renaming it to something like itemToken for clarity.
• Wrap the access-token, id, and uniquetoken query parameters with URL encoding to avoid issues if they contain special characters.
• Instead of using tostring in the JQ pipeline, use @json to serialize the forecast array into a proper JSON string.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `uniquetoken` parameter name overlaps with `token` and may be confusing—consider renaming it to something like `itemToken` for clarity.
- Wrap the access-token, id, and uniquetoken query parameters with URL encoding to avoid issues if they contain special characters.
- Instead of using `tostring` in the JQ pipeline, use `@json` to serialize the forecast array into a proper JSON string.

## Individual Comments

### Comment 1
<location> `templates/definition/tariff/solarprognose.yaml:42-44` </location>
<code_context>
+    default: mosmix
+    advanced: true
+  - name: interval
+    default: 1h
+    advanced: true
+render: |
</code_context>

<issue_to_address>
**suggestion:** Clarify supported interval formats to prevent misconfiguration.

If only specific interval formats are accepted, please validate or document the allowed values to avoid confusion.

```suggestion
  - name: interval
    description:
      en: Interval between forecast values (supported: "1h", "30m", "15m")
      de: Intervall zwischen Prognosewerten (unterstützt: "1h", "30m", "15m")
    choice: ["1h", "30m", "15m"]
    default: 1h
    advanced: true
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion: Clarify supported interval formats to prevent misconfiguration.

If only specific interval formats are accepted, please validate or document the allowed values to avoid confusion.

Suggested change

	- name: interval
	default: 1h
	advanced: true
	- name: interval
	description:
	en: Interval between forecast values (supported: "1h", "30m", "15m")
	de: Intervall zwischen Prognosewerten (unterstützt: "1h", "30m", "15m")
	choice: ["1h", "30m", "15m"]
	default: 1h
	advanced: true