An all too common occurrence that slows us down in the async world is
you make a PR, wait a day to get reviews
reviews come in, your code is good and works, but there are suggestions to make it better.
you refactor the code, re-write tests, etc, ask for reviews again, wait another day again.
This can go on for two or three iterations. My suggestion is simple, if it works decently well, you merge immediately, and create another WIP PR to make improvements (you have to create the PR and start working on it, otherwise it's just tech debt). Your new PR will include all good changes suggested by your team.
This will do two things.
1) It allows the code to reach the user and deliver benefits faster.
It may even validate/invalidate the entire feature. It may reveal a wrong approach, etc. Code on the IDE provides little information, code on prod reveals more information. The faster you receive information, the more feedback you have, the more agile you are. In the traditional sense of the word agile
2) Your next PR, the one fixing the tech debt, will be smaller, and different from the first one. It'll be easier to read and review. Sometimes you had good working code, and a well meaning suggestion could break your code or introduce hidden bugs. In that case, your original code is already on prod and working, and this single PR (the one adding the suggested changes) can be reverted.
Aim for fast feedback, increase surface area. Godspeed
Top comments (8)
This is ideal, but relies a lot on trust that sometimes can be abused by the reviewee. We have this issue at the company I work at where most people will follow up requested changes in subsequent PRs, but there are also some that don't, so how do you avoid that? Genuinely curious because it's something we're trying to solve without being too disruptive.
Could you just not create a sub branch of your feature branch and achieve the same thing? This would allow you to see the changes more clearly and is effectively the same but means you don't have to merge changes into your main branch that require further changes and thus keep main in a deployable state
We do trunk based, so we try to keep branching as flat as possible. I don't think management would want to change that for this issue. We're talking about it kind of loosely at the moment, but the sense I get is there needs to be some way of holding others to account. The situation I find that happens that holds up PRs for us is where someone wants to rush a feature out, quick and dirty, which is sometimes needed. The problem is how to ensure devs get the time to implement the feature correctly because product managers couldn't care less about the state of the code and want to move on to the next feature ASAP. Partly I think this is a culture thing and agreement between PMs,devs, and higher ups that sometimes time is needed to tidy up code so that the code base doesn't become a burden that takes longer to implement new features in the future.
You are absolutely right. Not to mention that sometimes refactorings and improvements in the way things are done can hide the main feature amidst all the changes, making it more difficult for a reviewer to understand.
In many cases, I feel like the first review, although boring, might get a lot of small problems in complex systems with a lot of overlap. Hence, I tend not to enjoy the suggested approach, although I feel like a PR with the fixes not of business logic problems but coding patterns, improvements, and so on is an amazing idea.
Of course, it can change depending on the project and company, I say this since I believe in previous companies I worked at this idea would be amazing
I agree with your point, but in my experience, it's better to take action in the moment rather than postponing it, because 'another time' often never comes. I believe it also depends on the team's culture.
If "in the moment" is actually in the moment, and not another cycle of 1 day of waiting for reviews, we agree :)
But it depends, async sometimes means different timezones, etc. We might want to move sooner rather than later. But yeah, if you can hop on a call and do it immediately, do it. If it'll take a day or half a day, I say create another PR, then merge the first one and focus only on the second
Tem algumas coisas que gostaria de ser mais específico
O que seria "if it works decently well", existem mil maneiras de fazer um omelete, você pode ser mais eficiente, menos eficiente, sujar mais panelas, enfim...
Neste ponto você tem razão
A visão que eu tenho um Pull Request é igual a um pedido que foi feita no restaurante, tem que chegar na mesa do cliente o mais rápido possível, entrentanto, os melhores pratos seguem padrões de apresentação, trazendo pra código, o código deve seguir tanto o padrão das boas práticas, como o padrão da equipe em geral.
Eu confesso que acho muito arriscada essa abordagem, pois também, você parte de uma premissa que todos do time tem um mesmo nível, e que não vão fazer códigos tão ruins nesse primeiro merge, coisa que pode não acontecer.
Prefiro a abordagem de revisão e tests primeiro, em sandbox, seguindo boas práticas, adotando padrões, do que revisitar uma coisa(claro que há uma deadline pra isso), e tem que haver um bom senso na questão do code review também.
Mas bom artigo Augusto!