PRs and code review

How do we get ported OH1 bindings reviewed and merged into 2.5.x? I’ve had a PR for Insteon queued up for over a month, trying to get it merged. It’s at https://github.com/openhab/openhab-addons/pull/6911

As far as I understood, only the core development for 2.5.X is halted, addons/bindings development is ongoing and updates/additions will still be accepted for the 2.X branch. I guess it just takes its time as reviewers are for the most part also core developers and pretty busy these days. Ping them on Github, not here.

1 Like

Hi Rob,

Your PR is absolutely the right place and right way, many thanks for it. @hilbrand also already did an initial review, so I would expect him to next also check that this comments are addressed in your updates. Note that we have >140 open PRs and far too few maintainers on the repo, so the reviewing queue is much longer than anybody wants it and it unfortunately takes time to process them. As @mstormi suggests, the best way is to regularly ping the maintainers on your PR to get some attention for it!

1 Like

How often is regularly?

Until they say “Don’t bug me” ?
I think every hour is too frequently. :wink:

3 Requests in the last 24 hours is far to much. Like @Kai wrote, there are a lot of PRs to be reviewed…

1 Like

If the requests were 8 hours apart, that seems pretty regular, by definition :wink:

2 Likes

@ranielsen It is possible to publish binding without being a part of primary OH codebase and lots of people did it during development. the Eclipse Marketplace is still in play and you can advertise and distribute your binary over there. Its possible that more people will spot it over there than over forums.
It brings some troubles down the path, but it also makes you free of long waiting in the PR queue.

Cheers!
Łukasz

1 Like

Maybe the @maintainers should spend some time getting PR’s reviewed and merged so they can be included in OH 3.0 conversion. Some of the PR’s have been sitting around for months.

1 Like

In other words, the end of 2021? That is my estimate for an actual stable release of OH3.

1 Like

It’s not my binding, it’s one that I ported over from OH 1 since nobody else has done it.

It’s yours now. :wink:

1 Like

Maybe you can pay my bills and I‘ll immediately start reviewing your contribution instead of going to work.

9 Likes

I think there is no space for criticism!
This is a beautiful tool absolutely no doubt. This is a great community. There are people spending there time and expertise for all of us.
It looks that there is no money driven idea behind.
And it is for free!!!
Also me I am getting sometimes angry that it isn’t easy to use, but so far I was always able to solve problems supported by the community.
And believe me most of the cases it isn’t only the tool but myself and lack of knowledge… right?
Also to understand but true this isn’t a tool for everybody, not the land of honey…
Ok I don’t want to spoil the thread, but I think it is important for such a community that from time to time someone repeats what we all already know,
And please proceed to develop and make it better and better, and I will try the pick the pieces of interest for me, and where I think I can manage, supported by the community…
and an automatic detection of Tasmota devices would be great:-)

3 Likes

All I’m asking is maybe a break should be taken from new development and time instead focused on open PR’s. Having 140 open PR’s is unhealthy for any project.

Just focusing on the oh1 migration tag, there are 15 of them. Because of this topic, I took the effort to port over the insteonplm binding, found testers to test the binding, but it sits idle waiting for review.

Also, the negative tone of the responses (including the one that was hidden) is exactly what has driven may people from wanting to contribute to the openhab project and have moved elsewhere. Is that what you want to happen?

2 Likes

I have one of those 15 (ecobee). I think I’m on my 8th or 9th binding at this point, 6 of which are in the distro and/or waiting to be included in the distro (globalcache, bigassfan, ambientweather, weathercompany, doorbird, ecobee). Some of the ones not in the distro are for my own personal use, and may never make it into the distro. The remaining ones are used by others by downloading the jar from my Github repo.

In my experience, I don’t think the time it takes to get a binding included in the distro is much different than it’s been for the last several years. I submitted a PR for my first binding in late 2016. It took 3 months for it to be merged. I submitted a PR for another binding in mid-2019. It took 3 months to be merged. So, from my perspective, not much has changed.

I never worried too much about the time it takes to get the binding included in the distro. The people who wanted to use my bindings always were able to get them.

Edit: I should add that things took a lot longer during the bnd migration. But that’s explainable.

5 Likes

Just four things:

  1. Please have a look at one of my own PR https://github.com/openhab/openhab-addons/pull/6678, which also sits there since December.
  2. I did have a quick look at the last month. I reviewed and merged 38 PR to 2.5.x in openhab-addons and reviewed some more that still wait for author’s response. In the same time 6 of my own PR were merged (4 bug fixes and one security related issue). I don’t think it’s fair to say that maintainers concentrate on development instead of review.
  3. No one wants to drive people away from this project. But: 14k lines PR requires at least a full day for review, maybe more. What I usually do when reviewing new contributions is first reading the doc to get an idea of what this is all about. Then I try to get an overview of the implementation (which is quite easy and maybe takes an hour or so for a 1-2k line PR but is much more complex for more than 6k lines). The last step is to review individual classes/files. It doesn’t make sense to split that up too much (at least for me), so: the larger the PR, the longer it takes to find a slot to make the review.
  4. Your code produces more than 200 compiler warnings and 286 warnings/infos of the code-analysis tool. Do you think it is fair to request me to review your code and make tons of remarks for things you could fix by looking at the output of the build?
4 Likes

Agree with Rob Nielsens post, there are far too many issues yet we seem to be steaming ahead releasing new versions and so on. More emphasis on closing existing issues would be fantastic!

2 Likes

As I said in the PR, this is bascially a port of the OH1 code as it said in the PR with minimal changes to get it running in OH 2+. A majority of the code is 100% identical to the insteonplm and shouldn’t need to be reviewed.