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

Acesso ao Sistema Expectativas #31

Closed
wants to merge 6 commits into from
Closed

Acesso ao Sistema Expectativas #31

wants to merge 6 commits into from

Conversation

angelosalton
Copy link

Este pull request implementa uma função para acesso aos dados do Sistema de Expectativas de Mercado do Banco Central.

  • Optei por criar um script separado para não causar confusão com as funcionalidades atuais do pacote.
  • Os testes unitários ainda não cobrem todos os casos possíveis de uso da função.

@rafpyprog rafpyprog changed the base branch from master to develop November 5, 2020 16:05
Copy link
Contributor

@opardal opardal left a comment

Choose a reason for hiding this comment

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

Refiz a revisão, conforme post abaixo.

Copy link
Contributor

@opardal opardal left a comment

Choose a reason for hiding this comment

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

Ola, @angelosalton

Tenho algumas sugestões para seu PR:

1) Arquivo __init__.py

Incluir o seguinte trecho para fazermos a referência:

from .expectations import expectations

2) Arquivo api.py

Não vejo necessidade de importar o módulo datetime. A atual versão deste arquivo já faz referência a função to_datetime do módulo common.py.

Portanto, incluir na linha de importação da common.py a função to_datetime. Resultando na seguinte linha de código:

from .common import LRU_CACHE_SIZE, MAX_ATTEMPT_NUMBER, to_datetime

Assim, sugiro alterar o seguinte trecho para referenciar a função já importada:

De:

 begin_iso = datetime.datetime.strptime(begin, '%d/%m/%Y').strftime('%Y-%m-%d')
 end_iso = datetime.datetime.strptime(end, '%d/%m/%Y').strftime('%Y-%m-%d')

Para:

 begin_iso = to_datetime(begin, 'pt')
 end_iso = to_datetime(end, 'pt')

3) Arquivo expectations.py

Remover a importação do modulo numpy.

4) Arquivo test_expectations.py

Remover a importação do modulo numpy.

Alterar o teste test_expectations_invalid_resource. A função, quando alimentada com um resource inválido, retorna um ValueError. Assim, sugiro alterar o teste para:

@pytest.mark.expectations
def test_expectations_invalid_resource():
    with pytest.raises(ValueError):
        expectations("foobar", "01/01/2020", "01/02/2020")

Qualquer dúvida, estou a disposição para qualquer tipo de auxílio. Abs.

Angelo Salton and others added 2 commits November 6, 2020 13:27
@opardal, obrigado pelas suas sugestões!
É meu primeiro PR aqui no pacote, futuramente revisarei os códigos com mais calma.

Co-authored-by: O Pardal <[email protected]>
@opardal
Copy link
Contributor

opardal commented Nov 6, 2020

Oi, @angelosalton

O seu PR não está passando nos checks ainda.

Veja, por favor, se modificando o arquivo __init__.py dentro do diretorio sgs resolve.

Sugestão: incluir o seguinte trecho:

from .expectations import expectations

* Update pytest.ini

* Update __init__.py

* Update api.py

Manipulando o caso em que a `end = None`

* Update api.py

Corrigido um erro no uso da função `to_datetime`

* Update api.py

Removida a possibilidade de utilizar `end = None`

* Update api.py

Retomando a abordagem com a biblioteca datetime.

A função to_datetime gera conflitos.

* Update api.py
@opardal
Copy link
Contributor

opardal commented Mar 20, 2021

@rafpyprog : Quando vamos incluir esse trabalho aqui? Acho que o @angelosalton vai ter que dar um rebase ou nao precisa?

@angelosalton angelosalton closed this by deleting the head repository Jul 21, 2023
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