Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exibe somente conteúdos com imagens no portlet de carrossel de imagens. #14

Merged
merged 1 commit into from
Oct 27, 2016

Conversation

idgserpro
Copy link
Member

Quando um usuário adicionava uma coleção, que possuia um conteúdo sem
imagem, no portlet de carrossel de imagens, ocorria o erro:

AttributeError: 'NoneType' object has no attribute 'url'

Agora exibimos somente conteúdos que com certeza possuem imagens.

Fixes #8

Quando um usuário adicionava uma coleção, que possuia um conteúdo sem
imagem, no portlet de carrossel de imagens, ocorria o erro:

    AttributeError: 'NoneType' object has no attribute 'url'

Agora exibimos somente conteúdos que com certeza possuem imagens.

Fixes #8
@idgserpro
Copy link
Member Author

@hvelarde , favor validar o pull request,
Grato

@hvelarde
Copy link
Member

para fazer minha vida mais simples, eu tivesse gostado não mixturar code analysis com a funcionalidade.

@idgserpro
Copy link
Member Author

@hvelarde você quer quando for code-analysis como commits diferentes, ou pull requests diferentes? Muitas vezes o erro de code-analysis ocorre no mesmo pull request de onde está sendo feita a demanda, portanto, não faz sentido ter um pull request que atende a funcionalidade e quebra a build, pra depois fazer outro pull request que só conserta o code-analysis ao nosso ver.

@hvelarde
Copy link
Member

@idgserpro PR diferentes, são coisas diferentes e mantendo o escopolimitado fica mais fácil de revisar e consume menos tempos. ai vamos mesclando mais rápido.

obrigado

@idgserpro
Copy link
Member Author

@hvelarde então faz um PR que quebra o build (pois precisa do code-analysis), você mescla um PR com o build quebrado, e depois faço um outro PR que conserta o build? Não estamos agindo de forma incorreta ao adicionarmos no workflow, de forma explícita, a mescla de builds quebrados?

@idgserpro
Copy link
Member Author

Em tempo: entendo o seu lado da revisão e obrigado novamente por nos auxiliar no processo. Só estamos achando esquisito adicionarmos a lógica de formalmente mesclar PR's com builds quebradas, sendo que isso deveria ser a exceção e não a regra.

@hvelarde
Copy link
Member

para evitar esses problemas podem ignorar os erros no code analysis e depois consertar na sequência num outro PR; ou podem fazer o contrário: consertar os erros de code analysis primeiro e depois consertar os bugs ou implementar as novas funcionalidades.

me desculpem a chatice mas estamos fazendo isso em horas vagas e sem pagamento nenhum.

@hvelarde
Copy link
Member

outra coisa: eu acho completamente desnecessário limitar as linhas em 80 caráteres; eu só faço isso quando realmente a linha é muito cumprida.

@idgserpro
Copy link
Member Author

me desculpem a chatice mas estamos fazendo isso em horas vagas e sem pagamento nenhum.

Sim, sabemos disso. Acho que podemos sim focar em por code-analysis antes da implementação em si pra facilitar a revisão.

eu acho completamente desnecessário limitar as linhas em 80 caráteres;

Facilita o diff numa variedade imensa de monitores, achamos bem útil, mas concordo em não deixar como obrigatório no code-analysis do pacote por ser uma questão de opinião.

Pergunta final: topa revisar desse jeito e focarmos nessa separação code-analysis e funcionalidade nos próximos pull requests? Esse pull request é pequeno, achamos que o esforço de separar agora em duas branches e dois pull requests não vale tanto a pena pelo tamanho.

Em tempo: no futuro pegaremos suas sugestões e incorporaremos usando o recurso de https://help.github.com/articles/creating-a-pull-request-template-for-your-repository/ para evitar que a gente cometa o mesmo equívoco no futuro (também serve para futuros colaboradores).

@idgserpro
Copy link
Member Author

@hvelarde Obrigado por ter concordado com nossas ponderações. Não temos mais opiniões sobre esse PR, se quiser mesclar pode fazê-lo.

@hvelarde hvelarde merged commit 9b66a14 into master Oct 27, 2016
@hvelarde hvelarde deleted the issues_8 branch October 27, 2016 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants