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.

Sign up for GitHub

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

Jump to bottom

[ntp] Clean up #7834

Merged

cpmeister merged 6 commits into openhab:2.5.x from lolodomo:ntp_nullannotations

Jun 19, 2020

Contributor

lolodomo commented Jun 1, 2020 •

edited

Loading

Add null annotations
Suppress one unused configutation setting (locale)
Get the default timezone from TimeZoneProvider
Map thing configuration and channel configuration to classes
Use constructor injection for the discovery service

Signed-off-by: Laurent Garnier [email protected]

lolodomo requested a review from marcelrv as a code owner

June 1, 2020 23:18

Contributor Author

lolodomo commented Jun 1, 2020

Binding fully retested with these changes.

Contributor Author

lolodomo commented Jun 2, 2020

I need to adjust the integration tests.


          [ntp] Clean up

04af195

Add null annotations
Suppress one unused configutation setting (locale)
Get the default timezone from TimeZoneProvider
Map thing configuration and channel configuration to classes

Signed-off-by: Laurent Garnier <[email protected]>

lolodomo force-pushed the ntp_nullannotations branch from 6f89095 to 04af195 Compare

June 2, 2020 07:20

Contributor Author

lolodomo commented Jun 2, 2020

Integration tests now ok again.

J-N-K requested changes

View reviewed changes

...binding.ntp/src/main/java/org/openhab/binding/ntp/internal/config/NtpThingConfiguration.java Outdated

+                  private static final int DEFAULT_REFRESH_NTP = 30;
+                  private static final int DEFAULT_SERVER_PORT = 123;
+                  public @Nullable String hostname;

Member

J-N-K Jun 2, 2020

Why don't you use something like

Suggested change

      
                public @Nullable String hostname;
          
                public String hostname = "0.pool.ntp.org";

You can skip the whole logic below then and directly access the fields in the code. If the user sets set parameter, the default is overwritten.

Contributor Author

lolodomo Jun 2, 2020

Ok, fixed.

...binding.ntp/src/main/java/org/openhab/binding/ntp/internal/config/NtpThingConfiguration.java Outdated

+                  private static final int DEFAULT_SERVER_PORT = 123;
+                  public @Nullable String hostname;
+                  public @Nullable Integer refreshInterval;

Member

J-N-K Jun 2, 2020

Suggested change

      
                public @Nullable Integer refreshInterval;
          
                public int refreshInterval = 60;

Contributor Author

lolodomo Jun 2, 2020

Ok, fixed.

...enhab.binding.ntp/src/main/java/org/openhab/binding/ntp/internal/discovery/NtpDiscovery.java Outdated

@@ @@ -44,12 +48,12 @@ public NtpDiscovery() throws IllegalArgumentException { @@
                   }
                   @Override
-                  protected void activate(Map<String, Object> configProperties) {
+                  protected void activate(@Nullable Map<String, @Nullable Object> configProperties) {

Member

J-N-K Jun 2, 2020

Why is this needed at all? It only calls the method it overrides

Contributor Author

lolodomo Jun 2, 2020

I should refactor this call and rather use injection.

Contributor Author

lolodomo Jun 2, 2020

Constructor injection done.

...g.openhab.binding.ntp/src/main/java/org/openhab/binding/ntp/internal/handler/NtpHandler.java Outdated

@@ @@ -54,217 +54,183 @@ @@
                * to one of the channels.
                *
                * @author Marcel Verpaalen - Initial contribution OH2 ntp binding
-               * @author Thomas.Eichstaedt-Engelen OH1 ntp binding (getTime routine)
+               * @author Thomas.Eichstaedt - Engelen OH1 ntp binding (getTime routine)

Member

J-N-K Jun 2, 2020

This looks wrong. Thomas's last name is "Eichstaedt-Engelen", so the spaces arounf the dash are wrong.

Contributor Author

lolodomo Jun 2, 2020

There was a warning on this line but my change was stupid.

Contributor Author

lolodomo Jun 2, 2020

Fixed

...g.openhab.binding.ntp/src/main/java/org/openhab/binding/ntp/internal/handler/NtpHandler.java Outdated

                   /** for publish purposes */
                   private DateTimeFormatter dateTimeFormat = DateTimeFormatter.ofPattern(DATE_PATTERN_WITH_TZ);
-                  private final LocaleProvider localeProvider;
+                  private @Nullable NtpThingConfiguration configuration;

Member

J-N-K Jun 2, 2020

Since all values have reasonable defaults, you can skip null checks by using

Suggested change

      
                private @Nullable NtpThingConfiguration configuration;
          
                private NtpThingConfiguration configuration = new NtpThingConfiguration();

Contributor Author

lolodomo Jun 2, 2020

Ok, done.

...g.openhab.binding.ntp/src/main/java/org/openhab/binding/ntp/internal/handler/NtpHandler.java Outdated

Comment on lines 106 to 107

		NtpThingConfiguration config = getConfigAs(NtpThingConfiguration.class);
		configuration = config;

Member

J-N-K Jun 2, 2020

Why not use

Suggested change

      
                    NtpThingConfiguration config = getConfigAs(NtpThingConfiguration.class);
          
                    configuration = config;
          
                    configuration = getConfigAs(NtpThingConfiguration.class);

Contributor Author

lolodomo Jun 2, 2020

I was to avoid null check warnings due to configuration which was nullable.
Now that configuration is non null, the local variable can be avoided.

Contributor Author

lolodomo Jun 2, 2020

Local variable removed.

...g.openhab.binding.ntp/src/main/java/org/openhab/binding/ntp/internal/handler/NtpHandler.java Outdated


		String timeZoneConfigValue = config.getTimeZone();

Member

J-N-K Jun 2, 2020

If you use the defaults directly, you can access the fields here and omit most of the null checks/function calls.

Contributor Author

lolodomo Jun 2, 2020

I can't define a default for the configuration because the expected default is the timezone provided by the TimeZoneProvider.

...g.openhab.binding.ntp/src/main/java/org/openhab/binding/ntp/internal/handler/NtpHandler.java Outdated

-                                  logger.debug("{} using default locale '{}', because configuration property '{}' is null.",
-                                          getThing().getUID(), locale, PROPERTY_LOCALE);
-                              }
+                              timeZoneId = TimeZone.getTimeZone(timeZoneConfigValue).toZoneId();
                           } catch (Exception e) {

Member

J-N-K Jun 2, 2020

Is it necessary to catch Exception here?

Contributor Author

lolodomo Jun 2, 2020

You're right, it is not necessary because it returns GMT if the string is invalid.
Fixed.

....openhab.binding.ntp/src/main/java/org/openhab/binding/ntp/internal/NtpBindingConstants.java Outdated

@@ @@ -37,14 +37,5 @@ @@
                   public static final String CHANNEL_DATE_TIME = "dateTime";
                   public static final String CHANNEL_STRING = "string";
-                  // Custom Properties
-                  public static final String PROPERTY_NTP_SERVER_HOST = "hostname";

Member

J-N-K Jun 2, 2020

I would prefer to keep the constants here, as that is what is most common in other bindings.

Contributor Author

lolodomo Jun 2, 2020

I often saw the constants defined in th econfiguration class too.
But ok, I revert.

lolodomo force-pushed the ntp_nullannotations branch from 888c605 to 18f34f2 Compare

June 2, 2020 23:18

Contributor Author

lolodomo commented Jun 2, 2020

java.util.TimeZone is no more used.
java.time.ZoneId is rather used.


          All review comments considered

c0e2029

Use constructor injection for the discovery service

Signed-off-by: Laurent Garnier <[email protected]>

lolodomo force-pushed the ntp_nullannotations branch from 18f34f2 to c0e2029 Compare

June 2, 2020 23:27

openhab deleted a comment from TravisBuddy

openhab deleted a comment from TravisBuddy

TravisBuddy commented Jun 2, 2020

Travis tests have failed

Hey @lolodomo,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

Contributor Author

lolodomo commented Jun 3, 2020 •

edited

Loading

java.util.TimeZone is no more used.
java.time.ZoneId is rather used.

I have to test again but it is possible that parsing of the timezone configuration setting using ZoneId.of(configuration.timeZone) is now failing in particular cases like for example "GMT+1:30" when there is no existing zone id. In that case, I will revert to TimeZone.getTimeZone(configuration.timeZone).

Contributor Author

lolodomo commented Jun 3, 2020

New timezone parsing is working well. In fact GMT+1:00 is an invalid value while GMT+01:30 is valid and correctly parsed by the binding.

Contributor Author

lolodomo commented Jun 3, 2020

And the full build is green.

openhab deleted a comment from TravisBuddy

openhab deleted a comment from TravisBuddy


          Pass ZoneId rather than TimeZoneProvider to the thing handler

7bf80ae

Signed-off-by: Laurent Garnier <[email protected]>

TravisBuddy commented Jun 8, 2020

Travis tests have failed

Hey @lolodomo,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

1 similar comment

TravisBuddy commented Jun 8, 2020

Travis tests have failed

Hey @lolodomo,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.


          Revert to keep TimeZoneProvider on the thing handler

4791b0e

Signed-off-by: Laurent Garnier <[email protected]>

Contributor Author

lolodomo commented Jun 9, 2020 •

edited

Loading

This is not yet good as TimeZoneProvider is considered only at thing handler initialization.
I will change that and test.


          Use TimeZoneProvider each time the timezone is required

7cc9c72

Signed-off-by: Laurent Garnier <[email protected]>

Contributor Author

lolodomo commented Jun 9, 2020

I tested how the binding is behaving with these changes when no timezone is set on the thing side but the user changes the openHAB system timezone. And this is working well, the openHAB system timezone is correctly taken into accound when the date/time is updated by the binding.

Contributor Author

lolodomo commented Jun 9, 2020

@J-N-K : could you please finish your review or more exactly review my changes after your first review ?


          Call activate in the discovery service constructor

3a6afd5

Signed-off-by: Laurent Garnier <[email protected]>

cpmeister approved these changes

View reviewed changes

Contributor

cpmeister left a comment

LGTM

Contributor

cpmeister commented Jun 12, 2020

@J-N-K can you take a final look?

Contributor Author

lolodomo commented Jun 18, 2020

Can we have this PR merged before 2.5.6 is released?

Contributor

cpmeister commented Jun 19, 2020

@J-N-K friendly ping

J-N-K approved these changes

View reviewed changes

cpmeister merged commit abc926d into openhab:2.5.x

cpmeister added this to the 2.5.6 milestone

lolodomo deleted the ntp_nullannotations branch

June 19, 2020 21:28

knikhilwiz pushed a commit to knikhilwiz/openhab2-addons that referenced this pull request


          [ntp] Clean up (openhab#7834)

284683f

* [ntp] Clean up

Add null annotations
Suppress one unused configutation setting (locale)
Get the default timezone from TimeZoneProvider
Map thing configuration and channel configuration to classes
Use constructor injection for the discovery service
* Use TimeZoneProvider each time the timezone is required
* Call activate in the discovery service constructor

Signed-off-by: Laurent Garnier <[email protected]>

CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request


          [ntp] Clean up (openhab#7834)

* [ntp] Clean up

Add null annotations
Suppress one unused configutation setting (locale)
Get the default timezone from TimeZoneProvider
Map thing configuration and channel configuration to classes
Use constructor injection for the discovery service
* Use TimeZoneProvider each time the timezone is required
* Call activate in the discovery service constructor

Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: CSchlipp <[email protected]>

MPH80 pushed a commit to MPH80/openhab-addons that referenced this pull request


          [ntp] Clean up (openhab#7834)

c1b82a0

* [ntp] Clean up

Add null annotations
Suppress one unused configutation setting (locale)
Get the default timezone from TimeZoneProvider
Map thing configuration and channel configuration to classes
Use constructor injection for the discovery service
* Use TimeZoneProvider each time the timezone is required
* Call activate in the discovery service constructor

Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: MPH80 <[email protected]>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request


          [ntp] Clean up (openhab#7834)

501dc15

* [ntp] Clean up

Add null annotations
Suppress one unused configutation setting (locale)
Get the default timezone from TimeZoneProvider
Map thing configuration and channel configuration to classes
Use constructor injection for the discovery service
* Use TimeZoneProvider each time the timezone is required
* Call activate in the discovery service constructor

Signed-off-by: Laurent Garnier <[email protected]>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request


          [ntp] Clean up (openhab#7834)

f21efba

* [ntp] Clean up

Add null annotations
Suppress one unused configutation setting (locale)
Get the default timezone from TimeZoneProvider
Map thing configuration and channel configuration to classes
Use constructor injection for the discovery service
* Use TimeZoneProvider each time the timezone is required
* Call activate in the discovery service constructor

Signed-off-by: Laurent Garnier <[email protected]>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request


          [ntp] Clean up (openhab#7834)

fce4f0b

* [ntp] Clean up

Add null annotations
Suppress one unused configutation setting (locale)
Get the default timezone from TimeZoneProvider
Map thing configuration and channel configuration to classes
Use constructor injection for the discovery service
* Use TimeZoneProvider each time the timezone is required
* Call activate in the discovery service constructor

Signed-off-by: Laurent Garnier <[email protected]>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request


          [ntp] Clean up (openhab#7834)

7a4b1cd

* [ntp] Clean up

Add null annotations
Suppress one unused configutation setting (locale)
Get the default timezone from TimeZoneProvider
Map thing configuration and channel configuration to classes
Use constructor injection for the discovery service
* Use TimeZoneProvider each time the timezone is required
* Call activate in the discovery service constructor

Signed-off-by: Laurent Garnier <[email protected]>

DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request


          [ntp] Clean up (openhab#7834)

1ed91e6

* [ntp] Clean up

Add null annotations
Suppress one unused configutation setting (locale)
Get the default timezone from TimeZoneProvider
Map thing configuration and channel configuration to classes
Use constructor injection for the discovery service
* Use TimeZoneProvider each time the timezone is required
* Call activate in the discovery service constructor

Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: Daan Meijer <[email protected]>

markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request


          [ntp] Clean up (openhab#7834)

ce70266

* [ntp] Clean up

Add null annotations
Suppress one unused configutation setting (locale)
Get the default timezone from TimeZoneProvider
Map thing configuration and channel configuration to classes
Use constructor injection for the discovery service
* Use TimeZoneProvider each time the timezone is required
* Call activate in the discovery service constructor

Signed-off-by: Laurent Garnier <[email protected]>

This was referenced Sep 23, 2020

[ntp] Reenable itest #8565

Merged

Fix disabled integration tests #8537

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet