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

[fmiweather] Fix discovery exception #17669

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented Oct 29, 2024

Fixes #17668
Refactoring similar to #17616

Fixes openhab#17668

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur added the bug An unexpected problem or unintended behavior of an add-on label Oct 29, 2024
@jlaur jlaur requested a review from ssalonen as a code owner October 29, 2024 22:17
@jlaur jlaur added the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Oct 29, 2024
@ssalonen
Copy link
Contributor

Thanks, looks reasonable to me!

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Thanks,LGTM

I need coffee, missed the test label. If there is a regression ping me and I’ll look at it with high priority

@lsiepel lsiepel merged commit 276254d into openhab:main Oct 30, 2024
5 checks passed
@lsiepel lsiepel removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Oct 30, 2024
@lsiepel lsiepel added this to the 4.3 milestone Oct 30, 2024
@jlaur
Copy link
Contributor Author

jlaur commented Oct 30, 2024

I need coffee, missed the test label. If there is a regression ping me and I’ll look at it with high priority

No worries, I needed glasses when looking for the "Convert to draft" in the GitHub app for Android. 😄 I just remembered that I wanted to have a look at possibly refactoring this, so that it wouldn't perform network call from the constructor, but rather when scan (manual or background) is triggered:

/**
* Creates a {@link FMIWeatherDiscoveryService} with immediately enabled background discovery.
*/
@Activate
public FMIWeatherDiscoveryService(final @Reference LocationProvider locationProvider,
final @Reference HttpClientFactory httpClientFactory) {
super(SUPPORTED_THING_TYPES, DISCOVER_TIMEOUT_SECONDS, true);
this.locationProvider = locationProvider;
httpClient = httpClientFactory.getCommonHttpClient();
stationsCache = new ExpiringCache<>(STATIONS_CACHE_MILLIS, () -> {
try {
return new Client(httpClient).queryWeatherStations(STATIONS_TIMEOUT_MILLIS);

I tested the forecast Thing, but not the observation Thing since I don't have such a weather station. @ssalonen - just to be sure, do you think you'd be able to test this?

@ssalonen
Copy link
Contributor

I am a bit short on time to test this out

it should discover observation stations nearby your location

so if you put helsinki's coordinates in openhab, you should see something popping up

@jlaur
Copy link
Contributor Author

jlaur commented Oct 30, 2024

it should discover observation stations nearby your location

so if you put helsinki's coordinates in openhab, you should see something popping up

Thanks, it works nicely! 👍

@jlaur jlaur deleted the fmiweather-discovery-exception branch October 30, 2024 19:36
KaaNee pushed a commit to KaaNee/openhab-addons that referenced this pull request Nov 8, 2024
Fixes openhab#17668

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Dec 16, 2024
Fixes openhab#17668

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[fmiweather] Exception on startup
3 participants