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

World's NIF #457

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

carolcardoso080
Copy link

Descrição

Mudanças Propostas

Checklist de Revisão

  • Eu li o Contributing.md
  • Os testes foram adicionados ou atualizados para refletir as mudanças (se aplicável).
  • Foi adicionada uma entrada no changelog / Meu PR não necessita de uma nova entrada no changelog.
  • A documentação em português foi atualizada ou criada, se necessário.
  • Se feita a documentação, a atualização do arquivo em inglês.
  • Eu documentei as minhas mudanças no código, adicionando docstrings e comentários. Instruções
  • O código segue as diretrizes de estilo e padrões de codificação do projeto.
  • Todos os testes passam. Instruções
  • O Pull Request foi testado localmente. Instruções
  • Não há conflitos de mesclagem.

Comentários Adicionais (opcional)

Issue Relacionada

Closes #<numero_da_issue>

@carolcardoso080 carolcardoso080 requested review from a team as code owners November 27, 2024 19:32
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.80%. Comparing base (d7b645d) to head (385803a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #457   +/-   ##
=======================================
  Coverage   99.80%   99.80%           
=======================================
  Files          19       19           
  Lines         521      521           
=======================================
  Hits          520      520           
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@carolcardoso080 carolcardoso080 changed the title 456 World's NIF Nov 27, 2024
@carolcardoso080
Copy link
Author

@camilamaia

Desculpa, a intenção não era já pedir para dar merge agora.
Estava seguindo o passo a passo do Contributing e acho que deu nisso.

Depois, se você conseguir dar uma olhada nos 2 códigos (de 2 países distintos), você me dá um retorno se está correto ou quais modificações seguir para manter o padrão!

Assim eu posso dar continuidade nos próximos da mesma forma.

@camilamaia
Copy link
Member

Oii @carolcardoso080!

Imagina, sem problemas! Se este pull request ainda não está pronto para ser mesclado e você deseja que ele seja apenas revisado ou analisado, é uma boa prática marcá-lo como rascunho (draft). Isso ajuda a sinalizar que o trabalho está em andamento e que o pull request ainda não está pronto para ser integrado.

Como converter um pull request existente para rascunho

  • Vá até o seu pull request.
  • Na barra lateral direita, na seção "Reviewers", clique em "Convert to draft".
image

Para instruções detalhadas, veja a documentação oficial do GitHub.

Como marcar um draft como pronto para revisão

Depois, quando você tiver já o seu PR pronto para realmente ser revisado para um merge, faça o seguinte:

  • Abra o seu PR
  • Desça até a caixa de mesclagem e clique em "Ready for Review".

image

Para instruções detalhadas, veja a documentação oficial do GitHub.

Como criar um pull request como rascunho

Caso já queira criar um PR como draft, no momento de criar o pull request, clique na seta ao lado do botão "Create pull request" e selecione "Create draft pull request".

image

Para mais detalhes, consulte a documentação oficial do GitHub.

Se precisar de ajuda nessa parte, é só chamar! 😊 Agora vou dar uma olhadinha no código!

Copy link
Member

@camilamaia camilamaia left a comment

Choose a reason for hiding this comment

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

@carolcardoso080 Aeee, muito obrigado pela contribuição! 🙌 E desculpa a demora para revisar — fim de ano foi uma correria por aqui. Mas agora consegui olhar com calma, e temos mais gente ajudando na manutenção do projeto, então vai ficar mais ágil. Bora daleee! É muito legal ver os primeiros passos saindo do papel!

Sobre a estrutura de pastas: você poderia mover a pasta worlds_nif para dentro da pasta brutils? Essa é a localização onde ficam as implementações dos utilitários. Assim, os caminhos corretos para os arquivos ficariam:

  • brutils/worlds_nif/nif-haiti.py
  • brutils/worlds_nif/nif-portugal.py

Vi que você até tinha feito exatamente assim antes, mas alterou depois. Aí fiquei sem entender hah

Fora isso, só comentei uns pequenos detalhes. Mas o código tá show, só continuar nessa pegada.

Ah, uma dúvida: o exemplo de uso está no arquivo apenas como rascunho por enquanto, certo? Imagino que a ideia seja mover essas instruções para os READMEs futuramente, correto?

Dá um toque se vocês precisarem de ajuda para continuar daqui, viu? Podemos marcar um pair se for útil para vocês.

Valeu!!

#############


def validate(nif): # type: (str) -> bool
Copy link
Member

Choose a reason for hiding this comment

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

Aqui, para seguir o padrão dos nomes de métodos, o nome deve ser is_valid. Ficaria assim:

Suggested change
def validate(nif): # type: (str) -> bool
def is_valid(nif): # type: (str) -> bool

#############


def validate(nif): # type: (str) -> bool
Copy link
Member

Choose a reason for hiding this comment

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

Mesmo esquema aqui, mudar o nome do método para is_valid:

Suggested change
def validate(nif): # type: (str) -> bool
def is_valid(nif): # type: (str) -> bool

@camilamaia
Copy link
Member

camilamaia commented Jan 13, 2025

@carolcardoso080 ah, percebi mais um detalhezinho agora vendo de novo. Os arquivos terminados em .py sempre vão utilizar o underscore _ para separação de palavras no projeto. Os arquivos vão ficar assim então:

  • brutils/worlds_nif/nif_haiti.py
  • brutils/worlds_nif/nif_portugal.py

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