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

handle weather forecast callback delay #586 #623

Merged
merged 3 commits into from
Feb 23, 2024
Merged

Conversation

afarago
Copy link
Contributor

@afarago afarago commented Jan 11, 2024

I have added pending temporary state - until the callback is not returned - to skip rendering any error messages in this state #586.

Please comment / modify if you see any better approach to circumvent the problem (e.g. delayed rendering).

Copy link
Owner

@decompil3d decompil3d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution. Can you please address the comments on the code and also add some tests to cover this adjusted functionality?

src/hourly-weather.ts Outdated Show resolved Hide resolved
src/hourly-weather.ts Outdated Show resolved Hide resolved
src/hourly-weather.ts Show resolved Hide resolved
handle if HA not supports subscription
@afarago
Copy link
Contributor Author

afarago commented Jan 22, 2024

Please check, I am ready with the requested changes.

Copy link
Owner

@decompil3d decompil3d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. The pending calculation looks better now, but I think we still need to adjust how we use this pending variable.

@@ -329,6 +331,7 @@ export class HourlyWeatherCard extends LitElement {
}

if (!forecastNotAvailable && numSegments > (forecast.length - offset)) {
if (pending) return;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about hiding this error in cases where the user has actually tried to configure the card to show more segments than the entity has available. I'm wondering if we should just make forecastNotAvailable be true if the forecast is pending.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is covered. Am I missing something?
When loading from the attribute (and subscription is available, yet not loaded) the error will be pending and will not be displayed.
Later on when the subscription returns and still number of segments are less than configured pending will be false, thus error is shown.

@decompil3d decompil3d merged commit af88f25 into decompil3d:main Feb 23, 2024
3 checks passed
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.

2 participants