PRs and code review

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. :roll_eyes:

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. :frowning_face:

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! :slight_smile:

(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

I agree. Very unkind that I value my own time at least the same as others.

Please don’t take my post out of context, Jan. My post didn’t quote you and wasn’t even directed at you.

I’ll take a look at that link, thanks!

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. :slight_smile:

That may have been a little harsh, but they were directed to the established guidelines and chose to complain rather than make the PR conform.

Yep. Very sweet, indeed. I’ve been using it ever since. :+1:

I don’t disagree and don’t say “throw the guidelines away, all shit”

Maybe take @higgers view for a moment

  • contributed to the project/community
  • took the effort to migrate an OH1 binding to OH2
  • everything is working fine
  • he tried to follow the guidelines
  • 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 :slight_smile: and yes I still like OH and contribute :wink:

1 Like

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.

I can relate a little bit to the frustration.

1 Like

BTW, @hilbrand added the comment to the PR on Jan 27th:

Thanks for picking this up. I did a quick scan of the code to give some initial review feedback.

I quickly fixed his feedback and then everything went dark. Just a suggestion, maybe more details on expected timelines on getting a PR approved.

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 :grinning:

1 Like

Did that involve adding 200 ignore/supress lines, or is there a better way?

Edit: looked at your PR and saw you can suppress once per class.

1 Like

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.

5 Likes

Thank you, Pope Kai :rofl:

2 Likes

@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.

1 Like