[pihole] Integration of API v6 #19350
Conversation
Signed-off-by: clinique <gael@lhopital.org>
...nhab.binding.pihole/src/main/java/org/openhab/binding/pihole/internal/rest/AdminService.java
Show resolved
Hide resolved
| import com.google.gson.Gson; | ||
|
|
||
| /** | ||
| * @author Martin Grzeslowski - Initial contribution |
There was a problem hiding this comment.
You are the author of this class, right?
|
Hey, thanks for the implementation of v6. I had a quick look on my cellphone, tommorow I will take a deeper look at it (using my pc and IDE). |
|
Thanks for you insights @magx2 . This is very early dev stage. My main concern is to find the equivalences between old API and the new one (I'm a recent user of Pi-hole). If there are not much, maybe I'll introduce a new Thing with a distinct channel set. Let me know your thoughts. |
Signed-off-by: clinique <gael@lhopital.org>
Signed-off-by: clinique <gael@lhopital.org>
|
@magx2 : I'm on a good track. Still searching equivalencies for some of the channel contents (null values line 88 of bundles/org.openhab.binding.pihole/src/main/java/org/openhab/binding/pihole/internal/rest/JettyAdminServiceV6.java). |
...nhab.binding.pihole/src/main/java/org/openhab/binding/pihole/internal/rest/AdminService.java
Outdated
Show resolved
Hide resolved
...nhab.binding.pihole/src/main/java/org/openhab/binding/pihole/internal/rest/AdminService.java
Outdated
Show resolved
Hide resolved
| */ | ||
| @NonNullByDefault | ||
| public interface AdminService { | ||
| public abstract class AdminService { |
There was a problem hiding this comment.
Why did you make it a class? i do not see any logic added
There was a problem hiding this comment.
Because it shares properties (and 1 method) between the two versions of the AdminService
| */ | ||
| @NonNullByDefault | ||
| public class PiHoleHandler extends BaseThingHandler implements AdminService { | ||
| public class PiHoleHandler extends BaseThingHandler { |
There was a problem hiding this comment.
Making AdminService a class required you to remove implements from here. It's not perfect cause adding new methods to JettyAdminService will not require you to expose those methods here.
There was a problem hiding this comment.
I did not see any obvious reason to expose these methods here. They are still available to actions.
| } | ||
|
|
||
| return null; | ||
| return PI_HOLE_TYPE.equals(thingTypeUID) |
There was a problem hiding this comment.
I'm not happy with this change. Adding more handlers will require you to get back to previous approach
There was a problem hiding this comment.
Sure, I'll revert.
bundles/org.openhab.binding.pihole/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
| <label>Server Version</label> | ||
| <description>Defines the API to be used with this server.</description> | ||
| <default>v5</default> | ||
| <advanced>true</advanced> |
There was a problem hiding this comment.
Are you sure it should be advanced? By default it will not be visible for the user
There was a problem hiding this comment.
They intent is not to bring any change to existing installations, but I can remove the advanced option for sure.
There was a problem hiding this comment.
Removing advanced will not change current installations, it will only make it easier for new installations
| * @author Martin Grzeslowski - Initial contribution | ||
| */ | ||
| @NonNullByDefault | ||
| public class JettyAdminServiceTest { |
There was a problem hiding this comment.
Don't forget to add tests for your service
Code review corrections. Signed-off-by: gael@lhopital.org <gael@lhopital.org>
Signed-off-by: gael@lhopital.org <gael@lhopital.org>
lolodomo
added
the
enhancement
|
Any updates? @clinique |
|
Hello @miloit , this PR is ready for review |
OH, I thought you are still working on it. Will take a look on it later this week |
4 participants
Early push to inform that work has started
Resolves issue #16852