-
Notifications
You must be signed in to change notification settings - Fork 460
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
Guia de contribuição #219
Guia de contribuição #219
Conversation
Poderia ter reaproveitado a PR #211 que está aberta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ótimo!
@williamespindola pode dar sua opinião para fazermos o merge? |
Muita coisa ja esta aqui, mas par complementar vale a leitura https://help.github.com/articles/setting-up-your-project-for-healthy-contributions/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gostei bastante, ele tem um caráter bem humano e informativo, o contrário do que eu propus que foi 100% técnico. :D
Mas senti falta de pontos técnicos, vou lista-los abaixo:
- Release Cycle ou Semver. Esta em definição ainda seu sei, mas não é informado aqui;
- Testes, sei que ainda não temos uma boa cobertura, mas se alguém vai contribuir precisa saber que precisa colocar, será uma condição sine qua non para seu PR ser aceito. E também nos polpa tempo no review, pois garante a funcionalidade.
- Não informa sobre documentação, se alguém adicionar uma feature nova precisa documentar isto.
- Um pull request por feature - Se você quiser fazer mais de uma coisa, envie várias pull requests.
- Se você tivesse que fazer vários commits intermediários enquanto desenvolvendo, or favor execute um squash antes de submeter. Com isto não vão precisar perguntar como fazer.
É isto, parabéns pelo trabalho, vamos em frente \o/
@@ -0,0 +1,19 @@ | |||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Legal pedir o ambiente aqui, algo assim:
## Seu ambiente
Inclua o máximo de informações relevantes sobre o ambiente que você encontrou o bug e como reproduzir isto.
- Versão usada (e.g. PHP 5.6, HHVM 3, I-educar 1.0):
- Sistema operacional e versão (e.g. Ubuntu 16.04, Windows 7):
- Navegador e versão (e.g Chrome 35.9.9.9)
- ...
- ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gosto! Vou colocar.
@@ -0,0 +1,9 @@ | |||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Não entendi muito bem este template, ele vai ser para features? Se sim poderia ter algumas informações como:
Contexto
Porque esta alteração é importante? Como você usaria isto?
Como esta alteração pode beneficiar outros usuários?
Possível implementação
Não obrigatório, mas sugira uma idéia de como isto poderia ser implementado.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Esse template vai ser pra todo o resto que não é bug report, então engloba features, refactors, etc. Achei que não valeria a pena fazer um template específico pra cada uma dessas coisas e espera-se que a pessoa tenha lido o guia antes de preencher a issue. Mas o texto que você usou é bem flexível e acho que pode ser aproveitado :) Vou tentar integrar da melhor forma possível.
|
||
Não se aplica. | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Colocando o template dentro de .github/ISSUE_TEMPLATE
ele automagicamente vai aparecer dentro do formulário qual alguém for criar uma issue, precisamos deixar aqui também?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Não entendi mto bem a pergunta, mas esse esquema funciona +/- assim:
https://github.com/TryGhost/Ghost/issues/new/choose
Eu nunca usei realmente e espero q tenha feito certo :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Animal isso!
CONTRIBUTING.md
Outdated
|
||
Outra ótima forma de contribuir é indicando melhorias ao código do i-Educar e em como ele está estruturado. Se você tem qualquer ideia de como podemos melhorar alguma abordagem na solução de problemas, refatoração de código, melhoria em algum recurso ou qualquer outra coisa relacionada, siga estes passos: | ||
|
||
1. Certifique-se de que sua ideia já não esteja sendo abordada em nosso [roadmap](./README.md#roadmap-de-tecnologia); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O Roadmap poderia ficar na wiki, um exemplo https://github.com/Respect/Validation/wiki/Respect%5CValidation-1.1
No readme é legal ter informações sobre o que é o projeto e como você pode instalar, executar, testar e informações legais.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Criei um planejamento a logo prazo aqui: https://github.com/portabilis/i-educar/wiki/Planejamento-T%C3%A9cnico
No readme pode ter esse de curso prazo e linkar o de longo prazo.
CONTRIBUTING.md
Outdated
1. Você realmente esta propondo uma ideia só ou um conjunto de ideias? | ||
2. Qual é o problema que sua ideia resolve? | ||
3. Por que sua sugestão é melhor do que o que já existe no código? | ||
4. Realmente vale a pena demandar tempo para implementar sua ideia dentro de nossas prioridades? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Estes pontos poderiam ficar no template para abrir a issue, falei sobre isto la no template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acho que o template é mais um negócio pra você preencher, pra tentar dar uma estrutura previsível de como os conteúdos vão chegar até nós. Nesse sentido, o seu comentário anterior me parece muito mais um template do que estes passos... Eu espero que a pessoa, antes de preencher uma issue, já tenha realizado estes passos e use o template só como guia pra preencher a issue de forma estruturada.
CONTRIBUTING.md
Outdated
|
||
Tendo passado pelo crivo de todos estes questionamentos basta [criar uma nova issue](https://github.com/portabilis/i-educar/issues/new) descrevendo as melhorias e usando o label **melhorias**. | ||
|
||
### Pedindo recursos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pedido de recursos e indicação de melhoria não seria a mesma coisa?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acho que recurso seria coisa realmente nova.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talvez os termos possam ser melhores? Realmente recurso é algo novo, que não existe no i-Educar hoje, e melhorias são ajustes ao que já existe... Alguma sugestão?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Para mim fica claro.
CONTRIBUTING.md
Outdated
- Seu código está completo ou próximo de estar completo; | ||
- Sua solução realmente funciona. Providencie testes se possível; | ||
- Seu código adere às convenções do [PSR-2](https://www.php-fig.org/psr/psr-2/); | ||
- Seus commits englobam bem as funcionalidades desenvolvidas. Evite WIPs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parece que estes três tópicos tem o mesmo objetivo, será que conseguirmos compilar em um único?
+- Seu código está completo ou próximo de estar completo;
+- Sua solução realmente funciona. Providencie testes se possível;
+- Seus commits englobam bem as funcionalidades desenvolvidas. Evite WIPs;
Se um PR foi enviado, ele tem que ser testado, mesmo o autor julgando estar pronto. A bateria de testes executada nos CIs e também execução local vai definir isto. Então quem estiver fazendo o review do PR que vai julgar se pode ser feito merge ou não.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acho que os dois primeiros itens podem se tornar um só. O último o que eu quis dizer é que, idealmente a pessoa deve fazer os commits atômicos, como você mesmo descreveu antes @williamespindola ... A ideia é não permitir que o PR venha com um monte de commits intermediários que não englobam bem as funcionalidades que foram desenvolvidas. Vou tentar reescrever ;)
CONTRIBUTING.md
Outdated
#### Sobre mudanças cosméticas | ||
|
||
PRs que realizam apenas mudanças cosméticas como remoção de espaços em branco, ajustes de indentação, etc., não serão aceitos. Nós valorizamos um código bem escrito e queremos padronizar nossas práticas, mas PRs que não entregarem nenhuma melhoria na estabilidade, funcionalidade ou testabilidade do projeto serão fechados. Para entender melhor sobre esta decisão veja [esta discussão](https://github.com/rails/rails/pull/13771#issuecomment-32746700). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Então, mas se eu fizer um PR agora para aplicar a PSR 2 eu vou estar ajustando a indentação, então ele não será aceito?
Acho que esta muito genérico, e vale exemplificar. Fiz um comentário sobre este tipo de PR na issue 208
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acho válido se está está no planejamento. Então PSR 2 pode ser aceito sim.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lembrar que PSR-2 é bem mais do que ajustar espaçamentos e tirar espaço em branco...
CONTRIBUTING.md
Outdated
- O código realmente resolve um problema real (de preferência baseado em alguma issue levantada); | ||
- Seu código está completo ou próximo de estar completo; | ||
- Sua solução realmente funciona. Providencie testes se possível; | ||
- Seu código adere às convenções do [PSR-2](https://www.php-fig.org/psr/psr-2/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Devemos aplicar o PHP-FIG como um todo e não apenas a PSR-2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acho que antes de obrigar tudo, o projeto deve ter tudo.
Etapa um seria o PSR1 e 2, então já temos que pedir isso aqui. Quando evoluirmos mais, vamos aumentando o grau de exigência. O que acha?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vejo o FIG como um standard para todo o projeto, é certo que vamos aplicar gradualmente, mas é bom aplicar de forma global.
Pode fechar a #211? Fechando... |
@williamespindola sobre os seus comentários:
Não adianta adicionar algo que não está maduro nem escolhido.
Não temos nem um ambiente de teste funcional ainda. Até o projeto obrigar testes, acho que vai um tempo. A Portabilis também não vai ter condições de escrever testes nesse momento pra tudo que for fazer.
Vamos abrir uma issue separada para isso, para não travar o PR.
Faz sentido!
Faz sentido! |
Refs #201 - Melhora e completa os templates para issues; - Adiciona esclarecimentos em alguns termnos no CONTRIBUTING.md; - Completa o CONTRIBUTING.md com mais detalhes técnicos; - Acrescenta planejamento de longo prazo (roadmap) ao README.md;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quero um PR cosmético... Quebra de linhas a 80 colunas para os .md
e .txt
pois facilita o diff
.github/ISSUE_TEMPLATE/outros.md
Outdated
|
||
**CONTEXTO:** | ||
|
||
Porque esta alteração é importante? Como você usaria isto? Como esta alteração |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Por que" para pergunta é separado :P
Refs #201 - Limita linhas em 80 caracteres no CONTRIBUTING.md - Typo em outros.md
Queridos. Acho que o @eberfreitas contemplou a maioria dos ajustes pedidos. Como amanhã o i-Educar vai ser relançado, pode ser que muitas pessoas entrem no repositório, e um direcionamento é bem importante. Então vou dar merge da versão atual, mas podem continuar discutindo possíveis ajustes aqui e criando issue do que pode ser melhorado. @williamespindola @farribeiro obrigado pelo review! |
Go! Go! Go! \o/ |
Agora que não pode fugir do roteiro... Agora não consigo fazer um simples OT ou Anuncio |
* 'master' of github.com:portabilis/i-educar: (23 commits) Corrigir link do Telegram Adicionando informações de comunicação no README Removido exclude Adicionado exclude paths EOF Create .scrutinizer Adiciona dependencias do esquema relatório que não estava presente no setup inicial do banco Corrigido ortografia Corrigido erros de ortografia Typos e ajustes; Ajustes discutidos no PR #219; Adicionando templates para issues; Versão inicial do CONTRIBUTING.md; Removido comentários Adiciona código de conduta no projeto Removido depreciado Removido script install_pear_packages.sh Removido script db.sh Apagado redundante Arquivo redundante ...
Proposta de guia de contribuição levando em consideração as discussões da issue #201 bem como o PR #211 e seus comentários.
Também contém sugestão de conteúdo que atente à issue #218.
Mudanças no README apontando para o guia, dentre outros pequenos ajustes.