Criando um bom Pull Request
Last updated
Was this helpful?
Last updated
Was this helpful?
Antes de abrir o Pull Request ou quando estiver fazendo um Code Review, execute um check-list com todos itens abaixo.
Se o Pull Request não atender algum dos requisitos definidos neste guideline, ele não pode ser mergeado.
A mensagem dos commits deve descrever de maneira clara o contexto/porquê daquela alteração. No artigo tem algumas boas práticas para escrever uma boa mensagem de commit.
A descrição do pull request deve conter o problema de negócio e técnico. Explicar a solução adotada, deixando bem claro porque da alteração.
Ter pelo menos um approve do mantenedor do time responsável;
Se o pull request for feito em par, o par não pode dar approve.
O pull request só pode ter uma responsabilidade, se conter mais, deve-se abrir em mais pull requests.
O pull request não pode ser mergeado se tiver algum -1 (ou change-request). Todos os comentários devem ser respondidos e/ou resolvidos.
O pull request precisa ao menos um approve, podendo ser ou não do mantenedor (podendo mudar por time);
Caso o mantenedor ou o responsável pelo pull request achar necessários mais approves, aguardar até que possua.
Caso por algum motivo a equipe precise de uma regra mais restritiva para os membros, o mantenedor (ou grupo de mantenedor) do time tem liberdade de colocar para o próprio time;
Pull request só é considerado aprovado quando o mantenedor der o approve, mas para ele dar o approve ele vai esperar a quantidade e qualidade de Code Reviews que ele julgar ser necessário para aquele PR.
Por exemplo, num pull request grande, o mantenedor pode esperar por ter mais code reviews para depois ele dar o OK. Num pull request pequeno de pouco impacto na aplicação, o mantenedor pode ele mesmo dar OK sem precisar de code review de ninguém.
Todo PR precisa de approve de apenas um mantenedor de cada contexto;
Se o time achar que não é necessário, não siga este ponto, apenas os do approve;
Caso por algum motivo o time precise de uma regra mais restritiva para os membros, o mantededor do time tem liberdade de colocar;
O mantenedor também é responsável pelo PR.
O código segue as convenções de código e arquitetura estabelecida pelo time?
O código está com a abstração correta? Se estiver utilizando herança, está utilizando ela corretamente?
O código está no lugar correto?
Quando existe mais de um padrão, o novo código está seguindo o padrão atual? O código está migrando para o padrão correto ou seguindo o padrão antigo?
O novo código não poderia reaproveitar código já existente? Está introduzindo código duplicado? Não poderia ser refatorado para um padrão reutilizável?
O código pode estar introduzindo problemas de segurança para a aplicação?
O código está adicionando funcionalidades que ainda não são necessárias? O código não pode ser simplificado?
Existem códigos não utilizados que poderiam ser removidos?
Quando adicionado uma nova dependência, realmente é necessário?
Exceções estão sendo tratadas corretamente?
Se foi criada uma nova dependência com um serviço externo, foi tomado o cuidado necessário para que uma eventual queda, lentidão ou mal-funcionamento do serviço externo não afete o sistema dependente?
O código pode estar introduzindo problemas de performance?
O código utiliza as estruturas de dados corretas para aquele cenário?
O código realiza muitas consultas custosas ao banco de dados?
Os nomes de classes, métodos, atributos, variáveis, parâmetros e afins refletem as coisas que elas representam?
Eu consigo entender o que o código faz lendo ele?
Eu consigo entender o que os testes fazem?
As mensagens de erros são entendíveis?
O código está fazendo o que ele supostamente deveria fazer?
Existem algum possível bug "oculto" como por exemplo checar uma variável errada ou utilizar um operador de comparação errado?
As mensagens de usuários estão escritas corretamente?
Existem erros óbvios que irão quebrar o ambiente de produção?
Os testes cobrem uma boa quantidade de cenários? Cobrem cenários felizes e casos de exceção? Existem casos que não foram considerados e deveriam ter sido considerado?
Existem testes que garantem que o código está realizando corretamente sua função? O teste está realmente testando os critérios de aceitação daquela funcionalidade?
O código segue boas práticas, como , e .
O código segue as convenções da linguagem? ( Exemplo para Java: , )