search

geekcom / validator-docs

Validação de CPF, CNPJ, CNH, NIS, DDD, Título Eleitoral e Cartão Nacional de Saúde com Laravel.
MIT License
566 stars 96 forks source link

Issue #111, problemas com campos não requeridos #116

Closed dimasf-dev closed 3 years ago

dimasf-dev commented 3 years ago

Ajuste feito para que sempre retorne válido qdo o valor não for informado, que é o caso do campo não ser requerido.

vicentimartins commented 3 years ago

Eu não concordo com um ponto deste PR: Quando o valor não for informado, por que precisa chamar o validate?! Esse valor precisa ser checado antes de chamar o validate.

@dimasf-dev posso contar contigo para refatorar esse PR? Sugiro utilizar o padrão strategy para esta implementação. Ao meu ver, se aplica melhor do que da forma que está implementado.

P.S.: É necessário que sejam implementados os testes referente aos trechos que estão sendo alterados.

dimasf-dev commented 3 years ago

Olá @vicentimartins, pode entrar em contato sim, sem problemas.

dimasf-dev commented 3 years ago

Desculpe a resposta anterior, li rápido e ache que estava querendo entrar em contato comigo, desculpe a forma como implementei, mas foi como eu pensei no momento, é a primeira vez q contribuo com código, se não tiver problema pra vc poderia refatorar para que eu possa ver como ficaria nesse padrão q vc citou?

Vou ver de implementar o teste.

Muito obrigado!

geekcom commented 3 years ago

Desculpe a resposta anterior, li rápido e ache que estava querendo entrar em contato comigo, desculpe a forma como implementei, mas foi como eu pensei no momento, é a primeira vez q contribuo com código, se não tiver problema pra vc poderia refatorar para que eu possa ver como ficaria nesse padrão q vc citou?

Vou ver de implementar o teste.

Muito obrigado!

Olá @dimasf-dev, obrigado pela contribuição, poderia verificar os erros que foram disparados logo abaixo, são referentes a PSRs. https://github.com/geekcom/validator-docs/pull/116/checks?check_run_id=2551416599

Outro ponto é que, apenas Pull Requests com testes são aceitos de acordo com nosso CI.