jaklan commented on PR #43453: URL: https://github.com/apache/airflow/pull/43453#issuecomment-2456597998
@potiuk > That was the part of first pass of review. At this stage assesment from me is that the change lacks unit test - so it won't be generally accepted anyway. We know it won't be accepted - the point is to get approval to finalize the PR. > Maybe. I have no idea, I am not a dbt expert. Possibly someone else who is will take a look. But a lot of people won't even look if there is no unit test, we generally don't accept PRs where unit tests work before and after the change - because it means that the change is not testable and prone to regression. > >Also unit tests help to guide reviewer in understanding what the change does - unit tests shows the scenarios in which the change gets used. Often unit test explain the reason and circumstances of the change better than words, and as such they are often used - especially when changes are small - as the review guideline. Also it shows that the author knows what they are doing since they are able to reproduce the situation they are talking about in unit tests. I don't see any value in writing unit tests before there's confirmation the approach is fine, otherwise it's just a waste of time of contributors. You start with design, not with coding (that's why the confirmation should happen first in the issue anyway, the PR was opened because you asked for it), and the reasoning is described in both the issue and MR description. It's more than enough to provide a feedback if it makes sense conceptually for someone familiar with given provider, especially in a situation when a few potential solutions were proposed. > There is no-one "responsible" - this is not how open-source work. There are > 3000 mostly volunteers - who similarl y as you - are both contributing and reviewing the code in their free time away from their day jobs and families, so the best course of action to get your PR merged, is to follow reviewer's comments and asks, and ping (in general not individual people) that your PR needs review. But there has to be someone responsible for the provider, who knows its specifics and who takes the final decision - I guess you don't merge PRs related to providers you don't know just because they seem to be documented and tested, you have to understand the impact. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
