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

Criação do Client.occurrences() #31

Merged
merged 16 commits into from
Oct 8, 2023
Merged

Conversation

FelipeSBarros
Copy link
Owner

@FelipeSBarros FelipeSBarros commented Sep 22, 2023

@cuducos comecei a criação do client.occurrences().

Na documentação são apresentados alguns exemplos de uso. eu fiz alguns testes e algumas coisas me chamara a atenção. Já me comuniquei com o Davi, desenvolvedor deles, para confirmar alguns pontos, como:

  • Só consigo acessar dados de uma cidade colocando o id dela E o id do estado. Ao informar apenas id da cidade, nada éretornado.
  • [logo,] Só são retornados dados se ao menos o id do estado for informado;

Em se confirmando que para acessar dados de ocorrência temos que informar o id do estado, seria o caso de levantar um erro, né?

Alguns pontos para trabalhar:

@cuducos
Copy link
Collaborator

cuducos commented Sep 22, 2023

Em se confirmando que para acessar dados de ocorrência temos que informar o id do estado, seria o caso de levantar um erro, né?

Creio que não. Ou pelo menos, não é dever nosso (de quem escreve o pacote) levantar esse erro. O que eu quero dizer é que isso tem que ser requisito da API. Se ID do estado é obrigatório, não podemos ter occurences(state_id=None) mas occurences(state_id).

Isso levanta um erro. Mas o que fazemos (nós) não é levantar o erro, é refletir no design da API a situação : )

@cuducos

This comment was marked as resolved.

@FelipeSBarros

This comment was marked as resolved.

@cuducos

This comment was marked as resolved.

@FelipeSBarros FelipeSBarros mentioned this pull request Sep 22, 2023
15 tasks
@FelipeSBarros

This comment was marked as resolved.

@cuducos

This comment was marked as resolved.

@FelipeSBarros

This comment was marked as resolved.

@cuducos

This comment was marked as resolved.

@FelipeSBarros

This comment was marked as resolved.

@cuducos

This comment was marked as resolved.

@cuducos
Copy link
Collaborator

cuducos commented Sep 23, 2023

Fiz uma requisição para states e peguei o id de cada UF. Depois peguei o pageMeta para as ocorrências de cada um deles:

{"page": 1, "take": 50, "itemCount": 39795, "pageCount": 796, "hasPreviousPage": false, "hasNextPage": true}
{"page": 1, "take": 50, "itemCount": 8866, "pageCount": 178, "hasPreviousPage": false, "hasNextPage": true}
{"page": 1, "take": 50, "itemCount": 2065, "pageCount": 42, "hasPreviousPage": false, "hasNextPage": true}

Digamos que cada requisição leve 0,5 segundo. Se formos retornar todas as ocorrências para a primeira UF, isso demoraria quase 7min. Acho uma péssima experiência para quem está utilizando. Melhor descartar a ideia de retornar todas as ocorrências de uma vez.

Pensei em talvez client.occurences() ser um gerador, que vai retornando ocorrências comforme quem utiliza quiser:

for count, occurence in enumerate(client.occurences(state_id=42), 1):
    print(occurence)
    if count == 51:
        break

Como a API retorna 50 por página, esse código geraria apenas duas requisições (ao invés de 796). Isso poderia ser implementado. Quando chamamos occurences() podemos verificar sem tem ocorrências já baixadas da API e não entregues a quem utiliza a API. Se tiver, devolvemos uma delas, sem necessidade de nova requisição HTTP. Quando esse buffer estiver vazio, vemos se tem próxima página. Se tiver, fazemos a requisção, devolvemos a primeira ocorrência e colocamos as restantes no buffer.

sequenceDiagram
    autonumber
    actor User

    User->>Occurences: next()

    rect rgb(239,224,209)

        Occurences->>Buffer: Checks if buffer is empty

        rect rgb(239,239,239)
            Note over Buffer, API Fogo Cruzado: Only when buffer is empty

            Buffer->>API Fogo Cruzado: HTTP request
            API Fogo Cruzado->>Buffer: HTTP response
        end

        Buffer->>Occurences: occurence
        Note over Occurences, API Fogo Cruzado: Generator
    end

    Occurences->>User: yield

ATUALIZAÇÃO Esqueci de mencionar, que o buffer pode ser só uma fila FIFO nativa:

In [1]: from queue import Queue

In [2]: q = Queue()

In [3]: q.put(1)

In [4]: q.put(2)

In [5]: q.put(3)

In [6]: q.get()
Out[6]: 1

In [7]: q.get()
Out[7]: 2

@cuducos cuducos changed the title inicia criação do client.occurrences() Criação do Client.occurrences() Sep 23, 2023
@cuducos cuducos changed the title Criação do Client.occurrences() Criação do Client.occurrences() Sep 23, 2023
@cuducos

This comment was marked as resolved.

@FelipeSBarros

This comment was marked as resolved.

@cuducos

This comment was marked as resolved.

@FelipeSBarros

This comment was marked as resolved.

@FelipeSBarros

This comment was marked as resolved.

@cuducos

This comment was marked as resolved.

@FelipeSBarros

This comment was marked as resolved.

@cuducos

This comment was marked as resolved.

@FelipeSBarros

This comment was marked as outdated.

@FelipeSBarros

This comment was marked as outdated.

@cuducos

This comment was marked as outdated.

@cuducos

This comment was marked as resolved.

Cria testes para a classe `Occurences`
@FelipeSBarros

This comment was marked as resolved.

Python/crossfire/crossfire/occurrences.py Outdated Show resolved Hide resolved
Python/crossfire/crossfire/occurrences.py Outdated Show resolved Hide resolved
Python/crossfire/crossfire/occurrences.py Outdated Show resolved Hide resolved
@FelipeSBarros

This comment was marked as resolved.

cuducos

This comment was marked as resolved.

@FelipeSBarros

This comment was marked as resolved.

@FelipeSBarros

This comment was marked as outdated.

Python/crossfire/crossfire/occurrences.py Outdated Show resolved Hide resolved
Python/crossfire/crossfire/occurrences.py Show resolved Hide resolved
Python/crossfire/crossfire/occurrences.py Outdated Show resolved Hide resolved
Python/crossfire/crossfire/occurrences.py Outdated Show resolved Hide resolved
Python/crossfire/tests/test_occurences.py Show resolved Hide resolved
Python/crossfire/tests/test_occurences.py Outdated Show resolved Hide resolved
Python/crossfire/tests/test_occurences.py Outdated Show resolved Hide resolved
Python/crossfire/crossfire/occurrences.py Outdated Show resolved Hide resolved
Python/crossfire/crossfire/occurrences.py Outdated Show resolved Hide resolved
Python/crossfire/crossfire/occurrences.py Outdated Show resolved Hide resolved
Python/crossfire/crossfire/occurrences.py Outdated Show resolved Hide resolved
Python/crossfire/crossfire/occurrences.py Outdated Show resolved Hide resolved
Python/crossfire/crossfire/occurrences.py Outdated Show resolved Hide resolved
Python/crossfire/tests/test_occurences.py Outdated Show resolved Hide resolved
Python/crossfire/tests/test_occurences.py Outdated Show resolved Hide resolved
Python/crossfire/tests/test_occurences.py Outdated Show resolved Hide resolved
Python/crossfire/tests/test_occurences.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@cuducos cuducos left a comment

Choose a reason for hiding this comment

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

🚀

@cuducos
Copy link
Collaborator

cuducos commented Oct 8, 2023

Algum motivo para o PR continuar como draft? Se não tiver, pode dar o merge na minha opnião : )

@FelipeSBarros
Copy link
Owner Author

Algum motivo para o PR continuar como draft? Se não tiver, pode dar o merge na minha opnião : )

uma revisão sua ;)
Entendo que posso dar o merge, então!

@FelipeSBarros FelipeSBarros marked this pull request as ready for review October 8, 2023 18:07
@FelipeSBarros FelipeSBarros merged commit ac2b2c9 into master Oct 8, 2023
2 checks passed
@FelipeSBarros FelipeSBarros deleted the add_client_occurrences branch October 8, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants