So far the testing shows the code is working great also after porting!
Wouldnāt it make sense to create a guideline how to prepare an efficient review? e.g.
- make sure that README includes all channels and a complete set of sample files
- check Travis logs and make sure to get rid of more or less all warnings
- Java 8 and 11 are green
- thing initialization runs asynchronous
- constructor injections are used
ā¦
3 Likes
Yes I also thought about that.
Looking at Home Assistant PRs, couldnāt we add some more automations which could help to get faster reviews/merges?
For example, a simple checklist, where all should ticked to get it merged (like in Home Assistant).
Also test coverage reports where tests are provided for a bundleā¦
Most of these things are already part of the developer documentation: https://www.openhab.org/docs/developer/
Constructor injection is missing (if I didnāt look wrong), but āno warningsā, āschedule of long-running initializationā are already part of it.
I check the page, but this not a checklist you should tick before submitting a PR for review or could please point me to the concrete info.
Perhaps he meant the next page here
Note that this list also serves as a checklist for code reviews on pull requests. To speed up the contribution process, we therefore advice to go through this checklist yourself before creating a pull request.
2 Likes
Openhab does have that, probably to a far greater degree due to Openhab being around a lot longer. In the case of the PR in question @J-N-K has reported that it has more than 286 warnings to address from the code analysis tool.
You will probably find that there is a link between code getting accepted more easily into Home Assistant and theā¦
- Number of breaking changes that annoy users.
- Number of PR being far greater than Openhab.
- Less stable then Openhab
Just recently I saw a maintainer over at HA state that the reason there was a breaking change was because a change had gone into the project that should not have been accepted and they then had to break things to fix the inconsistency after people had already started using the new features.
More code review means less breaking changes and more stability if the issues are found before they are included into the project, the downside is people then think it is a less popular project as they donāt see the huge changes made in separate projects before they get merged into Openhabs.
Huge thanks to all those who do contribute which includes @ranielsen for taking the time to port the older binding.
3 Likes
Pretty much all of the warnings are caused by new requirements for OH 2 where you must declare @NonNullByDefault. The code was ported over from OH1 and the errors donāt exist there. The following are examples that are causing warnings in eclipse. The code is fine:
Nullness default is redundant with a default specified for the enclosed type CommandHandler
@NonNullByDefault
public static class WarnCommandHandler extends CommandHandler {
WarnCommandHandler(DeviceFeature f) {
super(f);
}
dead code
private ConcurrentHashMap<String, InsteonChannelConfiguration> m_bindingConfigs = new ConcurrentHashMap<>();
InsteonChannelConfiguration bindingConfig = m_bindingConfigs.get(channelName);
if (bindingConfig == null) {
logger.warn("unable to find binding config for channel {}", channelName);
return;
}
Potential null pointer access: this expression has a ā@Nullableā type
@Nullable HashMap<String, @Nullable String> m_parameters = new HashMap<String, @Nullable String
protected int getIntParameter(String key, int def) {
String val = m_parameters.get(key);
if (val == null) {
return (def); // param not found
}
I can provide many more examples.
I feel for you as I am going through the same thing and some of the currently accepted bindings that you try and learn from fail the checks and they are already accepted into the project. As stated thank you for your work and donāt take any of the comments personally.
1 Like
Yea it sucks, looking at the info messages from the code analysis tool, it doesnāt like that the original developer used hungarian notation 
for example:
Name ām_featureā must match pattern ā^[a-z][a-zA-Z0-9]*$ā.
Changing names adds no value, and there is a good chance it will break something.
@ranielsen Hi Rob,
Iām not a maintainer but I reviewed your binding anyway.
Please stop pinging the maintainers now.
I need them to review my own PRs instead. 
PS: Good work on the migration btw.
1 Like
This is incorrect. Consistent naming adds huge value, and Hungarian notation hasnāt had any since the advent of modern IDEs.
I havenāt seen a good explanation for this, and I too find it silly.
3 Likes
+1
the result: @SuppressWarnings(ānullā)
I made a change to the fsinternetradio binding, ca. 15 lines of code + 100 driven by @NonNullByDefault. This turns a mini-PR into a mid-size one beside the fact that it is annoying to correct all the popping up warnings and finally there is still a left-over where Code Analysis has a different opinion than Eclipse and vice versa.
How to solve this one:
private Map<String, CoIotDescrBlk> blockMap = new HashMap<String, CoIotDescrBlk>();
CoIotDescrBlk blk = blockMap.get(sen.links);
if ((blk != null) && StringUtils.substring(blk.desc, 5).equalsIgnoreCase("Relay")) {
Eclipse reports āRedundant null check: The variable blk cannot be null at this
locationā for the " if ((blk != null)".
and there are many more.
1 Like
Exactly, There were 200 such warnings from the code analysis that I fixed but were replaced with warnings in Eclipse. Now thereās threat of not reviewing the code because of it. 
The old code is using consistent naming convention. I wouldnāt have picked Hungarian notation, but the original developer did.
I believe both of these issues were addressed in the dev links in this thread. The founding developer and his team made choices you do not agree with. The choice is to confirm or go elsewhere.
Home Assistant is different because addons run in their own isolated Docker containers, just using the API to interact with the core system.
Itās not that I donāt agree with them. I took it on myself to port a binding from OH1 to OH2 and thatās how it was written. It would require significant changes to code that has been stable for years in OH1.
The way this is going, Iām starting to regret my decision to take on the effort to help out the OH community by porting over a OH1 binding so it wouldnāt get left behind with OH 3. 
4 Likes
Please donāt take such an unkind comment to heart, nobody wants you to give up and go elsewhere. Without developers like yourself volunteering their free time to the OH weād all have a much poorer project to work with.
I can appreciate how frustrating it must be to take someone elses code, that was working in OH1, port it to OH2 and be told you have to wade through and fix tens or hundreds of (valid or not) issues with the code. However, as has been stated above, itās possible for people to install bindings manually. I and many others have done this with the IPCamera binding by @matt1 and the doorbird binding (before it was accepted into the OH distro) by @mhilbush. Iām sure there are many other bindings in a similar state. Iām sure many people will find great value in the work youāve done so far if you make the binding available to download manually somewhere.
Youāre doing great work, we all appreciate it. Donāt give up! 
(I would like to have a go at porting an OH1 binding in the future so Iām following your experience with interested. In fact, thereās value not only in the development work youāve but also in the discussion around it too.)
Just put it on Eclipse IoT Marketplace?
There hasnāt been just one, there have been multiple, including one that called me an _hole.
I used Tutorial: Migrating OH 1 addon to OH 2/3: preparing (1/x) to as a starting point which was helpful.
1 Like