[dwdpollenflug] Initial Contribution

[DerOetzi]

The "Deutsche Wetterdienst" (DWD) reports the current pollen count index for Germany on a daily base and a forecast for tomorrow and the day after tomorrow. This new binding allows you to retrieve this data for your region or partregion.

I added the information to this thread:
https://community.openhab.org/t/new-binding-dwdpollenflug/95596

Unfortunately I don't understand what I'm doing wrong at my git workflow. Can someone who knows about it have a look at my workflow description at #7289

[wborn]

Unfortunately I don't understand what I'm doing wrong at my git workflow. Can someone who knows about it have a look at my workflow description at #7289

I think you merged or resolved conflicts using the GitHub UI... or maybe merged changes locally? If you merge commits they will show up in your PR making it hard to review.

Instead you should rebase your branch to update it or resolve conflicts.

Here's a pretty accurate intro on how to do that using the command line:

https://dev.to/maxwell_dev/the-git-rebase-introduction-i-wish-id-had

It's also possible to do the same steps using the UI in Eclipse or any other IDE that supports Git.

[wborn]

I see you also wrote your steps in the issue. I think the missing part is the "force push" part. If you don't force it, it will not overwrite your existing branch... instead it will probably automatically make a merge commit for you. 😐

So it should have been:

git checkout dwdpollenflug
git rebase 2.5.x
git push --force-with-lease

You'll only need to force it after rebasing your branch on 2.5.x. After that you can use the normal push... until you rebase it again. Using the force option you can rewrite the history, whereas if you only add new commits these are linear, so in that case history doesn't need to be rewritten. :-)

You also don't have to switch to 2.5.x before you can rebase it.

This also works when you are on your dwdpollenflug branch:

git fetch upstream
git rebase upstream/2.5.x
git push --force-with-lease

You wouldn't even need a checkout of the 2.5.x branch in that case and you'll always be sure it doesn't contain any commits of your own that ended up on your local 2.5.x branch.

[DerOetzi]

@wborn thanks that was the problem. I'm always using shortcuts in my config and somehow the --force was lost in my shortcut file

[DerOetzi]

Just noticed that there are no reviewers entered on this PR. Have I to do something on my side to get my PR reviewed an merged?

[wborn]

I think you've found an issue because we need to update the fallback in the CODEOWNERS file since the team got renamed after the repo got renamed:

openhab-addons/CODEOWNERS

Lines 1 to 5 in 45cf759

	# This file helps GitHub doing automatic review requests for new PRs.
	# It should always list the active maintainers of certain add-ons.
	# As a fallback, if no specific maintainer is listed below, assign the PR to the repo maintainers team:
	* @openhab/2-x-add-ons-maintainers

[DerOetzi]

I think you've found an issue because we need to update the fallback in the CODEOWNERS file since the team got renamed after the repo got renamed:

openhab-addons/CODEOWNERS

Lines 1 to 5 in 45cf759

	# This file helps GitHub doing automatic review requests for new PRs.
	# It should always list the active maintainers of certain add-ons.
	# As a fallback, if no specific maintainer is listed below, assign the PR to the repo maintainers team:
	* @openhab/2-x-add-ons-maintainers

@wborn thanks

[DerOetzi]

As weather is getting better this days in Germany and with it the pollen pollution, is there a way how to promote this PR do get a quicker review?

[J-N-K]

Thanks for your contribution. I have left some comments inside.

[J-N-K]

I don't think you should add these properties, they just reflect the configuration.

[DerOetzi]

I removed region_id! The region_name and partregion_name I want to leave there for control reasons for those who use text-configuration, whether the correct region is loaded.

[DerOetzi]

Do you agree leaving region_name and partregion_name as properties?

[cpmeister]

Why is this needed if users can just trigger on changes to the "Update Time" channel?

[DerOetzi]

I thought about use case like: don't need to link a item to the refresh channel because I don't want to display anywhere but just want to trigger a rule on refresh

[DerOetzi]

Removed trigger channel!

[cpmeister]

The first letter of every word in a label should be capitalized.

Suggested change

	<label>Update time</label>
	<label>Update Time</label>

Please update elsewhere as appropriate.

[DerOetzi]

Is this an OpenHAB convention? Should I although do it like "Refresh In Minutes" or like "Refresh in Minutes". And should it be the same in translation file?

[J-N-K]

Using upper-case letters as first letters in labels is an openHAB convention, yes. This also applies to translations. Please do not use something like "In Minutes". If the unit is set in the description, the UI should properly display the unit.

[DerOetzi]

Fixed all labels to upper case and removed in Minutes

[cpmeister]

You at least handle a RefreshType command

[DerOetzi]

Just for my understanding of this, when ist RefreshType command triggered?

[DerOetzi]

Please check my fix

[cpmeister]

Suggested change

	final ScheduledFuture<?> localPollingJob = this.pollingJob;
	if (localPollingJob == null || localPollingJob.isCancelled()) {
	final ScheduledFuture<?> localPollingJob = this.pollingJob;
	final DWDPollenflug pollenflug = this.pollenflug;
	if (localPollingJob == null || localPollingJob.isCancelled()) {

[DerOetzi]

please check my fix.

[cpmeister]

Instead of 'e', please give the variable a name that at least gives a hint as to what the datatype is.

[DerOetzi]

Please check my fix.

[DerOetzi]

@J-N-K and @cpmeister I have refactored the complete DTO package. I would ask you to review it again. Thanks

[DerOetzi]

And I just noticed a problem with translation.

    <channel-group-type id="updates">
   	<label>Updates</label>
   	<description>Information about data state</description>
   	<channels>
   		<channel id="refreshed" typeId="update">
   			<label>Bridge refreshed</label>
   		</channel>
   		<channel id="last_update" typeId="update">
   			<label>Last update from DWD</label>
   		</channel>
   		<channel id="next_update" typeId="update">
   			<label>Next update from DWD</label>
   		</channel>
   	</channels>
   </channel-group-type>

The labels of the channels are not translated. I thought it should work like this:

channel-group-type.dwdpollenflug.updates.refreshed.label = Letzte Aktualisierung Bridge
channel-group-type.dwdpollenflug.updates.last_update.label = Letzte Aktualisierung durch den DWD
channel-group-type.dwdpollenflug.updates.next_update.label = N\u00E4chste Aktualisierung durch den DWD

[J-N-K]

Thanks for your fast response. Already looks much better. I'm glad we already got @cpmeister as a second reviewer. That'll speed up the whole process.

[TravisBuddy]

Travis tests were successful

Hey @DerOetzi,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[TravisBuddy]

Travis tests were successful

Hey @DerOetzi,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[cpmeister]

LGTM

[DerOetzi]

@cpmeister Thank you as well for the help