PRs and code review

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

This thread cracks me up - I’ve had a PR waiting nearly 4 years now!! Of course, it’s waited that long because of me rather than anyone else :wink:

The problem is - it’s by far the largest binding now and I have very dim hopes it will ever be reviewed/accepted by anyone (will take too much time). @cpmeister did a great job of reviewing and probably spent way too much time doing an initial review - much thanks to him!

1 Like

Could you please share a link to that PR.

1 Like

It’s the sony one - latest pr is https://github.com/openhab/openhab-addons/pull/6884
Original PR is (way back in 2016): https://github.com/openhab/openhab-addons/pull/1249

Some stats: 322 files (most of them are DTOs), 52k lines (code+doc+whatever), resulting jar file is 1.8MBish in size - it’s huge :wink:

EDIT: link to the forum we discuss this binding with: Sony Devices Binding

So your post was not a complaint ?

Heavens no - just was laughing since mine has been around forever (and it’s all due to me)…

1 Like

Ok, fine then.

I’d definitely want it merged for 3.0, though. I hope we can make that @tmrobert8!