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.
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.)
Maybe not the right attitude rather than running an open discussion!
That’s how this could end up
Ok, then we should be respectful and accept other opinions.
We should be open to experience from people contributing to the project finding the right balance between unified code style (which makes a lot of sense and also the code analysis helped me to find some errors) and dealing with annoying stuff like “Travis vs. Eclipse warnings”. Maybe a revised template for the style checker could help, like I submitted the PR to let files *DTO.java accept NunNullByDefault. I reported the issue, it was discussed by the maintainers, I created the PR, merged and yup got rid of 150 warnings.
but is faced with 250 warnings, which
a) is a lot of work to get rid off
b) the changes could have some side effects
And this is not OH1->OH2 specific, I had the same trying to implement 15 lines of code for the fsinternet binding ending up in a PR with +229 and -122 lines, whereas the actual change is around 20 lines = even for a reviewer it looks like a bigger job that it is.
So it’s not the matter of “he doesn’t want to follow the guidelines and is just to lazy to produce a PR conforming to those”.
Nothing should be written in stone. I’m not a Java expert, the only thing I could say is
@NonNullByDefault drives me nuts (and obviously more people)
creates frustration
reduces my willingness to support porting other bindings to OH2/OH3
I’m doing Shelly, MagenaTV, Rachio, AppleTV so… bites me more than once.
All said from my side and yes I still like OH and contribute
I spent many hours manually entering literally hundreds of options on the first Z-Wave device I entered into OH. The manufacturer made a flexible device with many options and they each needed to be entered manually. We then needed to wait for binding feature additions that took months.
Kai chimed in on my PR and said I could add annotations to ignore the 200+ warnings that @NonNullByDefault created. After adding the annotations, I was left with a single warning for a deprecated constructor which I fixed as well. Now there are no compiler warnings
Just for the record: That’s an exceptional approval for code that is moved from the OH1 binding, it is not a general absolution to everybody to not use null annotations and ignore the warnings. Also for migrated OH1 bindings, newly created classes (like the ThingHandlers) should definitely have null annotations.
@ranielsen That is great news. I have been testing the new binding and it is working well. Thank you for your efforts on this. Hopefully we can get this into a release in the near future.