Skip to content
This repository has been archived by the owner on Dec 6, 2019. It is now read-only.

Buscar transações por período #29

Closed
wants to merge 6 commits into from

Conversation

@lcobucci
Copy link
Member

lcobucci commented Jul 2, 2015

@fabiopaiva muito obrigado por sua colaboração!

Tenho mais alguns pedidos a lhe fazer 😄

  1. Todos os arquivos DEVEM estar de acordo com a PSR-2, execute o vendor/bin/phpcs --standard=PSR2 src tests para ver a lista de correções;
  2. Crie testes de unidade para as classes que você está inserindo por favor.

Obrigado mais uma vez!

@fabiopaiva
Copy link
Author

Hehe, bom que mantém a qualidade do código. Daqui a pouco mando.

@fabiopaiva
Copy link
Author

@lcobucci quais testes a mais são necessários?
O teste LocatorTest::getByPeriodShouldDoAGetRequestAddingCredentialsData abrange todas classe que eu inseri, não é o suficiente?

public function decode(SimpleXMLElement $obj)
{
//criar transações
$transactions = array();
Copy link
Member

Choose a reason for hiding this comment

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

Estamos preferindo o uso do short-array syntax, pode alterar pra ficar padronizado por favor?

@lcobucci
Copy link
Member

lcobucci commented Jul 3, 2015

Sobre os testes @fabiopaiva, você está fazendo um mock do seu Decoder, portanto se for inserido um erro nele nós não saberíamos.

O ideial é sim usar mocks para as dependências, porém também adicionar testes para novas classes (seria fodástico se TODAS as classes tivessem seus testes, mas um dia a gente chega lá).

Mais uma vez muito obrigado pela força!

P.S.: Depois das alterações faz, por favor, um rebase do seu branch pra deixar o histórico lindão (se não souber fazer manda aí que a gente ajuda 😄).

@lcobucci
Copy link
Member

lcobucci commented Jul 3, 2015

By the way, você tá fazendo as interações com o user @fabiopaiva, mas os commits estão com o @fabaopaiva. Acho que seria legal você deixar em apenas um pra tudo aparecer no perfil que você tá usando no github.

@fabiopaiva
Copy link
Author

Tem como trocar o autor dos commits?

@fabiopaiva
Copy link
Author

Pronto, já corrigi e o commit tá indo certo agora. Como faço o rebase?

@lcobucci
Copy link
Member

lcobucci commented Jul 7, 2015

@fabiopaiva parece que o email que você fez os últimos commits não foi identificado no github.

A ideia do rebase é reescrever o histórico do seu branch e assim reorganizar as coisas. Utilizamos ele pra sincronizar o branch de origem com o de destino (quando necessário) e também para ajustar os commits do branch.

Para este segundo caso usamos o git rebase -i $idDoCommitAnteriorAsSuasMudancas. Esse ID pode ser visto através do git log. Seu editor irá abrir com a lista de commits, e você poderá escolher por editar seu histórico (no editor terá instruções). Após ajustar as coisas o git irá aplicar as alterações e, como você já enviou seus commits para o servidor central (github), você deverá executar um git push -f para enviar forçadamente o novo histórico do branch.

No seu caso a ideia seria juntar todos os seus commits em apenas um, assim teremos um histórico mais limpo.

Mais informações podem ser encontradas em https://robots.thoughtbot.com/git-interactive-rebase-squash-amend-rewriting-history e https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History.

P.S.: Não esqueça de ver o lance do TransactionSearchItem pra ver se não conseguimos reutilizar coisas e também os testes de unidade para as classes e métodos que você inseriu, por favor!

@fabiopaiva
Copy link
Author

@lcobucci, confere se agora está ok.

$service->getByPeriod($initialDate, $finalDate, $page, $maxPageResults)
);

$realDecoder = new Decoder();
Copy link
Member

Choose a reason for hiding this comment

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

Isso não deveria estar aqui. Cada classe tem que ter seu teste, este é o do Locator, você deve criar um teste para o decoder.

@lcobucci
Copy link
Member

Tá chegando lá @fabiopaiva!

Desculpe ser tão chato, mas a ideia não é só adicionarmos as features, mas sim deixar tudo direitinho em cada lugar.

Obrigado pela paciencia e pela colaboração, espero que este processo esteja te ajudando também.

@lcobucci
Copy link
Member

@fabiopaiva não desistiu não né cara?

@fabiopaiva
Copy link
Author

Rsrs não. Tempo mesmo

@lcobucci
Copy link
Member

Hhahahaha pensei que tinha te assustado =P

 Author:    Fábio Paiva <fabio@paiva.info>
@fabiopaiva
Copy link
Author

Que nada, aprendi muita coisa de Git, valeu a pena demais. Olha agora se está ok

@fabiopaiva fabiopaiva reopened this Jul 19, 2015
/**
* @test
*/
public function getByPeriodShouldDoAGetRequestAddingCredentialsData()
Copy link
Member

Choose a reason for hiding this comment

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

Esse método ficou quase 100%, o $this->transactionSearchResult só é utilizado neste teste, portanto podia ser configurado apenas pra ele (variável local ao invés de atributo da classe).

Copy link
Author

Choose a reason for hiding this comment

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

ok

@lcobucci
Copy link
Member

Estamos quase lá 😄

Falta fechar os itens dos comentários que fiz e também inserir o teste do Decoder (no seu próprio arquivo de testes).

Precisa de ajuda nisto?

@fabiopaiva
Copy link
Author

Tudo bem, vou tentar arrumar um tempinho aqui, essa semana estou muito ocupado.

@lcobucci
Copy link
Member

De boa, se precisar de alguma ajuda dá um berro

@fabiopaiva
Copy link
Author

"inserir o teste do Decoder (no seu próprio arquivo de testes)"
Não entendi. O único método que adicionei ao Decoder foi o decodeTransactionSearch e este é testado.

@fabiopaiva
Copy link
Author

@lcobucci confere agora por favor.

@lcobucci
Copy link
Member

Tá ficando bom o negócio @fabiopaiva!

O lance dos testes é o seguinte: quando a gente trabalha com testes de unidade cada classe deveria ter o seu arquivo de teste, e os testes de cada classe não deve depender da implementação de outras classes (por isso que usamos os test doubles - conhecidos também por mocks).

Você criou o teste da TransactionResult, mas esse teste está verificando o comportamento do Decoder, sacou? Aqui tem algumas informações interessantes: https://phpunit.de/manual/4.7/pt_br/writing-tests-for-phpunit.html e http://xunitpatterns.com/

@mborgesmartins
Copy link

@lcobucci como ficou esse processo? Essa interface é muito importante. Seria bem interessante que fosse concluída. Acho que o amigo @fabiopaiva ficou meio sem tempo e essa implementação parou. Sem pressão, mas vocês chegaram tão perto...

@fabiopaiva
Copy link
Author

Eu fui estudar um pouco mais e também sem tempo...
@lcobucci esta funcionalidade ainda está pendente, certo? Vou retomar os ajustes para finalizar, ok?

@wcomnisky
Copy link
Member

@mborgesmartins fique à vontade para fazer um fork e colaborar!

@lcobucci
Copy link
Member

@lcobucci esta funcionalidade ainda está pendente, certo? Vou retomar os ajustes para finalizar, ok?

@fabiopaiva tá sim cara, se vc puder continuar será fodástico!

@sergiors
Copy link
Member

sergiors commented May 2, 2017

@fabiopaiva, alguma novidade? :P

@lcobucci lcobucci closed this May 28, 2017
@lcobucci lcobucci changed the base branch from develop to master May 28, 2017 19:37
@lcobucci lcobucci reopened this May 28, 2017
@sergiors
Copy link
Member

ping

@mrprompt mrprompt added enhancement Tests missing This issue or pull request doesn't have a test case labels Nov 7, 2018
@cassiosantos
Copy link
Member

@fabiopaiva muito obrigado pela sua contribuição!

Infelizmente, como este PR é de 2015 e não tem atualizações desde 2017, vou encerá-lo.

Devido a falta de disponibilidade dos colaboradores também estamos descontinuando este projeto.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Tests missing This issue or pull request doesn't have a test case
Projects
None yet
7 participants