“Era uma vez uma revisão de código, escrevendo comentários pelo correio, indicando o número da linha. Foi muito engraçado. Dos profissionais: ninguém olhou para nada no diffs, olhou no IDE. Mas também havia um ponto negativo: após alguma fusão, os números das linhas mudaram. "
Alexander Makarov, Yii
“Nossa empresa tem um conceito interessante - um pedido de cadeira . É quando, dentro do mesmo escritório, um desenvolvedor se aproxima de você em uma cadeira e diz: 'Olha, isso é mais rápido do que criar uma solicitação de pull.'
Anton Morev, WormSoft
Recentemente, houve uma gravação pública do podcast SDCast sobre revisão de código no YouTube. Selecionamos e deciframos os mais interessantes da edição.
Versão de áudio completa no Spotify, Ya. Music e como um arquivo para download
Versão de vídeo completa no Youtube
Não trate as revisões de código como verificar algo ou procurar bugs
Sergey Zhuk, Skyeng: Acredito que o objetivo fundamental de uma revisão de código é compartilhar conhecimento dentro da equipe e encontrar a melhor solução. A equipe analisa as alterações propostas - e o conhecimento sobre o domínio é distribuído igualmente entre seus membros. O autor da solução entende como o código que ele considerava perfeito pode realmente ser melhorado.
Portanto, as revisões de código não devem ser evitadas ou feitas mais rapidamente. Este é um investimento no negócio e na equipe: o código fica melhor, a equipe fica melhor. Sim, no início não gostei quando o pedido foi finalizado (e quem gostaria disso).
Mas então comecei a ver isso como um processo de aprendizagem, a par de ler livros ou participar de conferências.
Eu senti isso além de mim mesmo, como um desenvolvedor, cresci com essa abordagem.
Mas há uma nuance. Certamente muitos de vocês já se depararam com solicitações de mil linhas, em que a refatoração e um recurso e algumas pequenas alterações foram misturadas. Ao misturar coisas diferentes em uma solicitação, complicamos tanto o compartilhamento de conhecimento quanto a vida do revisor: será mais difícil para ele avaliar se o comportamento do sistema mudou, se o requisito do negócio foi atendido, se o problema como um todo foi resolvido - e a solução foi bem-sucedida?
Percebemos esse momento em nossa equipe e introduzimos uma regra: em uma solicitação de pull, não interfira em mudanças de outra natureza. Você envia refatoração separadamente, recursos separados e pequenas alterações separadamente.
Relatório de Sergey sobre as práticas de sua equipe. A versão em texto está aqui .
Essas solicitações geralmente são analisadas de forma rápida e fácil e têm muito mais probabilidade de receber feedback de qualidade. E da parte do mantenedor, há vantagens: se você gosta de refatoração, e o recurso tem um bug, então você pode mesclar a refatoração separadamente.
Alexander Makarov: Concordo que você não deve tentar gastar o menos tempo possível em revisões de código. Aberto, como se os colchetes estivessem bem, esse código faz alguma coisa - não funciona dessa maneira. Se o revisor não puder garantir o código até o final, isso significa que a revisão do código não foi realizada.
Portanto, agora começamos a compilar um número bastante grande de diretrizes para nós mesmos, e uma delas fala sobre a revisão do código.
Parte das diretrizes da equipe Yii.
Mas, para a tese sobre pequenas solicitações individuais de pull: Eu tive situações em que um lead veio e apresentou algo assim. A equipe era hostil. Como você conseguiu isso?
Sergey Zhuk: Havia uma equipe em paralelo com a qual interagimos, usamos a API deles. Eles fizeram um monte de recursos em um mês, nós fizemos apenas um pouco. Ou seja, inicialmente não nos concentramos na entrega de recursos, mas na qualidade com esta abordagem. E combinamos com a empresa que vamos lançar mais devagar, mas procuramos não quebrar nada. Um mês depois, um dos vizinhos quebrou, depois outro. Mas nós não. E neste exemplo, todos entenderam tudo.
Konstantin Burkalev, SDCast:Eu tinha processos para implementar revisões de código em geral - e não era fácil, embora todos parecessem entender que era bom, sim. Você diz: "Pessoal, vamos fazer a fusão por meio de uma solicitação de pull". Eles dizem: "Sim, há um pequeno recurso."
O princípio aqui realmente funciona bem: quando algo quebra, você diz: “Mas se você fizesse um pedido, nós teríamos olhado e simplesmente não chegamos a esse ponto”. A ideia é que as pessoas entendam os erros por experiência própria. Tentar impor nem sempre funciona.
Com que rapidez revisar
Durante a transmissão, a votação ocorreu entre o público. Aqui está um deles.
Konstantin Burkalev: Junho é especialmente assim: “Bem, você atendeu ao meu pedido, está tudo bem aí? Eu escrevi, cerrei os punhos e esperei ”.
E eu tenho minha própria tarefa, só posso chegar lá à noite ou não sei ...
Sergey Zhuk: Em nossa equipe, a revisão sempre foi uma prioridade. Portanto, existe um regulamento - quando chega uma solicitação pull, você termina o que está fazendo agora, para não perder o contexto, e vai para a revisão. Porque o que você está prestes a fazer ainda está em processo. E essa tarefa já foi realizada.
O código já foi escrito, resta apenas olhar, mesclar e fazer upload para o produto.
Ou seja, terminei algo meu, troquei, olhei rapidamente e continuei a trabalhar. Provavelmente, 3 vezes por dia me distraio de minhas tarefas para revisão. Mas, novamente, você precisa entender que em minha equipe tudo é dividido em pequenas solicitações de pull, de 200 a 300 linhas de código cada. Conseqüentemente, um mínimo de tempo é gasto.
"Quem revê é menos importante do que quem"
Konstantin Burkalev: Isso levanta a questão - quem deve revisar. Apenas chumbo? Apenas o senor? Ou pode e deve ser dado a alguém com menor competência, apenas para que a pessoa tente crescer?
E quando perguntadas sobre o que revisar, as pessoas responderam assim.
Alexander Makarov: Se você tem muitas pessoas em sua equipe e o líder criou um gargalo em si mesmo, isso retarda a equipe inteira e, como resultado, torna a equipe muito pior. Como líder, quando eu trabalhava na Skyeng, eu tinha 15 solicitações de pull por dia em seu pico, e não as menores. Reservei um tempo para a revisão pela manhã e à noite.
É necessário revisar todos. Exceto, provavelmente, para histórias em que "fogo, tudo está pegando fogo" - não vai ficar pior lá.
Estou bagunçando tudo, está tudo bem - embora eu use o mesmo projeto por muitos, muitos anos. Portanto, agora estou tentando convidar o número máximo de pessoas para assistir ao meu pedido. Isso é bom e deveria ser.
É outra questão se todos devem confiar na revisão. Eu tinha caras que podiam descobrir muito, mas eles tinham grandes problemas com o foco: e, digamos, um passou 6 horas em uma revisão. Tive que ensinar as pessoas a administrar seu tempo.
Se estamos falando de empresas comerciais, sou a favor de identificar quem tem essa responsabilidade e quem pode revisar à vontade - e quanto tempo por dia você pode despender na revisão, dependendo desse status.
Anton Morev:A meu ver, existem processos diferentes. Se estamos fazendo algum recurso que precisa ser lançado em pouco tempo, não podemos deixar que June veja o código. Mas se estamos fazendo algum tipo de produto interno ou o prazo não é alto, sim, é uma ótima prática deixar June revisar o que o desenvolvedor líder ou sênior fez. Assim, os Juns vão entender melhor o que está acontecendo no projeto como um todo e como funciona o mecanismo de tomada de decisão.
"Este é um rejeitado imediatamente"
Sergey Zhuk: Skyeng implementou uma verificação para uma mensagem de confirmação: deve haver um número de tarefa no JIRA, caso contrário, a solicitação de pull não pode ser mesclada. Às vezes, você abre o código, mas não entende o que está lá - você abre uma tarefa e pode entender algo.
Quanto ao resto, a princípio foi difícil, mas decidimos rejeitá-lo apenas se a tarefa fosse completamente errada ou se a equipe discordasse arquitetonicamente. E então: colocamos uma aprovação, mesclamos, escrevemos um comentário - e se o autor da solicitação pull quiser, ele retornará e corrigirá. Lá, ele corrigirá pequenas rugosidades ou usará algum tipo de padrão. Quais são as suas práticas de rejeição?
Alexander Makarov: Os critérios coincidem completamente com os seus - se a tarefa não for bem executada, é claro, você tem que finalizá-la. Mesmo que o código pareça bom e arquiteturalmente, tudo é legal.
, : , .
Por exemplo, dizemos: “Gente, vamos fazer um teste”. Há exceções, é claro, mas os testes são muito importantes. É claro a partir deles se a pessoa entendeu a tarefa corretamente: novamente, se ela testa classes e métodos, e não um caso de uso, isso já é suspeito. Agora acabamos com a infecção , isso é um teste de mutação. Tulza sai dos mesmos testes, mas modifica o código e executa. E se o código alterado for aprovado nos mesmos testes, parte do código não será necessária - você pode simplesmente pegá-lo, excluí-lo, nada mudará.
Um pouco de backstage.
Além disso, é claro, problemas de segurança e desempenho - haverá uma rejeição aqui. Não rejeitamos ninharias: ou pedimos para consertá-los, ou nós mesmos os consertamos - empurramos diretamente para as solicitações de pull daqueles que os fizeram e já mostramos no código final o que, como e por que o fizemos.
Anton Morev:Em relação ao que você disse - o problema está resolvido? Acontece que um desenvolvedor, ao resolver um problema, resolveu outro. Não é necessário rejeitar tais situações. Ou pelo menos descobrir como a tarefa foi moderada.
Konstantin Burkalev: Mas o momento de vincular mensagens de commit a um rastreador de tarefas parece importante para mim. Existem tarefas do dia a dia em que ajuda muito. Você sabe quando: "Escute, fizemos algo parecido com o problema do botão." Você encontra a tarefa sobre o botão, entende o número, vai ao log do repositório, encontra o diff desses commits e vê: de fato, nós estragamos isso e aquilo, pode ser movido aqui.
Mas o oposto também acontece. Estou olhando o código-fonte de um projeto aqui e me deparei com a função action684 no back-end.
Escrevo para um amigo, pergunto, que nome legal é esse no código. Ele responde - uma referência à tarefa no rastreador. “Por que inventar um nome para uma função, você está escrevendo como parte de uma tarefa” ... Thresh, que não deveria existir em um mundo normal, é claro.