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

Contributing #211

Closed
wants to merge 1 commit into from
Closed

Contributing #211

wants to merge 1 commit into from

Conversation

farribeiro
Copy link
Contributor

@farribeiro farribeiro commented Jun 7, 2018

Contribuiting e outras coisinhas

Closes #201

Copy link
Contributor Author

@farribeiro farribeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Para revisão

CONTRIBUTING.md Outdated

- **Document any change in behaviour** - Make sure the `README.md` and any other relevant documentation are kept up-to-date.

- **Consider our release cycle** - We try to follow [SemVer v2.0.0](http://semver.org/). Randomly breaking public APIs is not an option.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A intenção é adicionar a este tópico o que coloquei na issue #199 de Release Cycle. Mas precisamos definir la primeiro. O que coloquei la segue o SemVer v2.0.0

composer.json Outdated
"config": {
"vendor-dir": "ieducar/vendor"
},
"require": {
"php": "~7.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um dos motivos que ainda não tinha feito o PR era porque eu ainda não sabia qual era a versão do PHP que o projeto esta rodando, você sabe @farribeiro?

"psr-4": {
"Portabilis\\Ieducar\\": "tests"
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O projeto não esta usando namespace, então não tem problema ficar estes caminhos nos autoloaders. Logo que começar a migrar para namespace vai começar a encostar nisto.

@farribeiro
Copy link
Contributor Author

farribeiro commented Jun 8, 2018

Se não me engano tem um problema no seu commit ... Nele você altera um JSON

@williamespindola
Copy link
Contributor

O composer.json?

@farribeiro
Copy link
Contributor Author

farribeiro commented Jun 8, 2018

Yep

Faça alterações necessárias para que este PR seja aceito

@williamespindola
Copy link
Contributor

Done, composer atualizado e ja aproveitei para configurar o code sniffer junto do scrutinizer, ja podemos ter algumas métricas do que não esta seguindo o contributing.

@farribeiro
Copy link
Contributor Author

Pode ser mergeado assim mesmo?

@MarceloCajueiro
Copy link
Contributor

Galera. Sei que ainda não temos regras e boas práticas definidas, mas esse PR tem muita coisa aberta.

Alguns pontos:

  • @farribeiro acho complicado você abrir pull request de algo que não é você que está trabalhando.
  • Uma boa prática é ter pequenos commits separando o que cada um faz
  • Uma outra boa prática é abrir PR de assuntos específicos. Ex.: Adiciona code sniffer, Configura scrutinizer, Adiciona doc de contribuição.

O doc de contribuição o @eberfreitas está trabalhando duro nele. Depois podemos adequar com o que foi feito aqui.

Outra coisa é que nosso público alvo é o Brasil e muitos profissionais de TI que forem olhar o projeto podem ter dificuldade no Inglês, então o ideal é documentação escrita em Português.

Sugiro fechar esse PR e reabrir com assuntos específicos.

@farribeiro
Copy link
Contributor Author

farribeiro commented Jun 9, 2018

Então será fechado e propõe a mesma PR do branch, que tive a iniciativa de propor o trabalho alheio.

Lembre-se disso #201 (comment)

@farribeiro farribeiro closed this Jun 9, 2018
@farribeiro farribeiro reopened this Jun 9, 2018
@farribeiro
Copy link
Contributor Author

farribeiro commented Jun 9, 2018

@MarceloCajueiro levar em consideração que o mantedor do branch, @williamespindola , considera Ok para merge!

@williamespindola dê o tique verde aí

@@ -0,0 +1,19 @@
filter:
excluded_paths:
- tests/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acho que devemos adicionar aqui:

- 'ieducar/tests/*'
- 'ieducar/misc/**/*'

"fabpot/php-cs-fixer": "Fix PSR2 and other coding style issues"
},
"autoload": {
"psr-4": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adicionar isso não quebra nada o projeto?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Não deveria porque hoje vocês não estou usando o autoload, estão fazendo require_once de todas as dependências. Vou dar uma olhada mais afundo.

@@ -1,15 +1,48 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as mudanças no composer, nada aqui pode impactar o código de produção?

CONTRIBUTING.md Outdated
```


**Happy coding**!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acho que esse arquivo pode ser removido. Vamos achar a melhor versão para isso na issue.

Seria legal esse PR cobrir apenas ferramentas que garantem o mínimo de qualidade, sem quebrar o projeto.

@@ -0,0 +1,19 @@
filter:
excluded_paths:
- tests/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acho que não adianta colocar os testes para rodar se o atual não funciona. Colocar test: false seria bom.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Não seria legal manter para ver o quando progredimos na aplicação dos testes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Até podemos, mas até a primeira implementação de teste sair vai ficar red.

@farribeiro
Copy link
Contributor Author

Aplique ASAP para não termos maiores problemas com o scrutinizer

- Arquivo com guia e regras para contribuição
- Refatora arquivo composer.json com novas dependências e configurações
- Adiciona como dependência fabpot/php-cs-fixer para chegar o estilo do
código
Copy link
Contributor

@williamespindola williamespindola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@farribeiro bora separar este Pr em partes diferentes? O de contributing vamos deixar para o #219 eu ja levantei o que enviamos neste relacionado a contributing que esta faltando la. E o de la ficou melhor que o meu, o meu ficou muito técnico.

Acho que podemos separar em 2:

  • Adiciona code sniffer, adicionando o arquivo de configuração .php_cs e o composer.json
  • Configura scrutinizer, apenas o arquivo .scrutinizer.yml

Vamos apenas definir as questões que o @MarceloCajueiro levantou ali e enviar os PRs.

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.

3 participants