PR Rules : Relax requirement for 1PR=Single platform for new integration PRs

asantaga commented 4 years ago

Context

Context : https://developers.home-assistant.io/docs/creating_component_code_review#5-make-your-pull-request-as-small-as-possible

Within the PR rules it says that each PR should only contain one platform. I agree this should be the case for most, almost all cases, however if someone has been developing their integration they may have used a couple of platforms to achieve something which could be considered needed for MVP. In my case I created an integration over 18 months ago, distributed it via HACS, and evolved it to be something very useful using 3 platforms and about 5-6 services. Once I realised the integration is a) popular and b) useful I started looking to put it into the core HA (more visibility and contribute to the project) but Im being told I need to submit multiple PRs , probably one at a time.

My integration PR is https://github.com/home-assistant/core/pull/36805

Proposal

I do not think we should relax the rule of one platform per PR EXCEPT for the initial PR of any integration. If a developer is submitting a new integration then that should be allowed to have multiple platforms.

Consequences

By making this change we will :

a) Make moving a integration from HACS to core HA easier, one PR not multiple
b) Reduce the burden on the integration developer where they have to split up the integration into multiple PRs
c) Reduce the burden on the HA reviewer. there will be one PR instead of multiple PRs each one submitting something "piece-wise"
d) Both of the above allow the developers time to work on something else.

thanks for listening :-)

Kane610 commented 4 years ago

The whole point of that rule is to get small PRs. That will speed on the review and increase the likelihood of it getting approved and integrated. Check the apple tv PR as an example having multiple platforms and have been available in HACS during development it is still open :(.

tetienne commented 4 years ago

Being a popular HACS integration does not mean it fit the HA quality rules. The reviewers can dislike the way you wrote someone because not homogeneous with code base for instance. It's why small PRs are really important.

asantaga commented 4 years ago

Being a popular HACS integration does not mean it fit the HA quality rules At the same time it doesn't mean that it doesn't fit the quality rules - To prep it for HA core I worked to get it up to spec.. But the quality rules are not the issue here, its the limitation of the initial PR only allowing one platform..

jjlawren commented 4 years ago

Several smaller incremental PRs that build upon each other are far easier to understand and review effectively than a monolithic one. The total time to review will be much shorter if it's broken into logical sections. Large and complex PRs shift the effort onto the reviewer(s).

Don't read it as a "rule" but a helpful recommendation to save the limited time of the author and reviewers.

balloob commented 4 years ago

Review 2000 lines PR. Find an issue.
Contributor fixes it.
Review 2000 lines again.

Now not only do we have to review a lot of lines again and again, the chance that we find issues increases too! So we have more often have to review all the code to make sure it still all works. And that is not even considering that if I set aside X time for reviewing, a 2000 line PR generally does not fit in.

frenck commented 4 years ago

It is not just the size of the PR, but there is general logic that is mostly the same for all platforms. Often, if a PR has multiple platforms, the same "mistakes" and thus review comments are made multiple times in all of the platforms.

Keeping things small and simple, is easier to review and iterate on as a developer. In the end, the throughput is way higher with smaller and PRs with a limited scope. Which in the end, everybody benefits from.

Furthermore, the larger part of all the work done on the Home Assistant repositories is voluntary. Nobody has to review a PR... Large PRs are usually skipped by volunteers, nobody likes to review those.

So in conclusion, nobody benefits from them. The biggest loss of large PRs? The end-user, not getting a new integration or feature.

As for my personal opinion, I vote against relaxing the rule. If anything, I would vote for making them even stricter.

asantaga commented 4 years ago

ok,

I'll submit a few PRs this weekend,

home-assistant / architecture