-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Habilidade de selecionar um item específico #178
Conversation
de API: https://localhost:8000/45.543.915/0001-81/uf https://localhost:8000/45543915000181/uf
Adicionalmente, seria bacana se você incluir um link com o meu nome direcionado ao meu github. |
Oi @EduApps-CDG, antes de mais nada, obrigado pela sugestão de ideia e de implementação. Eu tenho algumas questões com essa sugestão. Primeiro, do problema que ela resolve:
Isso não é um gargalo relevante atualmente e a #167 resolveria isso de forma muito mais abrangente, com testes e mais possibilidades para quem for utilizar a API. Sobre o PR em si, acho que é teríamos que pensar em algumas coisa antes de prosseguir:
E acho que aí faltaria mãos, já que considero #175 urgente e nem disso estou dando conta : ( ATUALIZADO Com um ponto que esqueci de mencionar:
|
Olá, vou providenciar os testes e estas outras questões assim que eu chegar em casa. Estamos com a necessidade de retornar dados específicos, então vou dar mais atenção a esta PR quando possível. Também vou dar uma olhada neste #175.
|
Não entendi essa parte. Mas enfim… todo mundo que colabora com o projeto automaticamente aparece aqui. |
Estava pensando em algo mais assim: Mas fica a critério seu |
O problema é mais os dados que podem ser considerados desnecessários em alguns casos. Mencionei o uso de rede apenas como um adicional, embora aqui não seja algo muito relevante. |
Um detalhe: o que é o arquivo |
Também me pergunto o que seria este arquivo, parece ser algum tipo de artefato. Achei que fazia parte do projeto então não mexi nele. |
Não faz parte do projeto. Você que o adicionou a esse PR no commit f085363. Isso é um problema: não queremos esse arquivo que não sabemos o que é no histórico do Git do projeto, então apenas |
…os requests" This reverts commit f085363. Changes to be committed: modified: api/api.go deleted: oryxBuildBinary
Só pra avisar, foi modificado o código fonte. Agora para acessar um item específico você pode usar este link:
Também removi o arquivo. Ainda falta eu criar os testes. Em seguida darei uma olhada no outro issue. Irei gerar o commit ainda hoje, se possível. |
Changes: modified: api/api.go modified: api/api_test.go
Changes: modified: api/api_test.go
api/api.go
Outdated
json.Unmarshal([]byte(s), &data) | ||
|
||
//if command contains ",", it means that the user wants multiple fields | ||
if strings.Contains(command, ",") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me pergunto se if
é necessário. Tenho a impressão que se o command
for razao_social
, strings.Split(command)
funcionaria igual: https://go.dev/play/p/Yqn_OiOxgM_t
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tem razão. Vou remover este if.
api/api_test.go
Outdated
http.MethodGet, | ||
"/19.131.243/0001-97?fields=uf,cep", | ||
http.StatusOK, | ||
`{"cep":"","uf":""}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O que acha de usar, nesse caso de teste, campos que tem valores em testdata/response.json
? Algo como data_inicio_atividade
e descricao_porte
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claro, incluirei as mudanças!
api/api_test.go
Outdated
{ | ||
http.MethodGet, | ||
"/19131243000197?fields=xolofompila", | ||
http.StatusNotFound, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Não acho que 404 Not found
descreva a situação aqui, pois o recurso existe. O problema é a requisição solicitando coisa que não existe. Acho que é o caso de um 400 Bad request
. O que você acha?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pode ser, na minha cabeça eu estava pensando "o campo não foi encontrado".
Deixei alguns comentários isolados, depois faço um review mais formal quando tu disser que terminou essa leva de edições. Se quiser um direcionamento no que eu senti nessa leitura:
Sendo bem pragmático: talvez o esforço aqui desse PR seja melhor aproveitado ajudando @vmesel nessa issue do que implementando algo que potencialmente vai ser descartado em pouco tempo, sabe? |
Changes modified: api/api.go modified: api/api_test.go
Vejo isto se tornando um pull request gigante. Mas respondendo sua pergunta, a redução do uso de rede é um adicional. O problema que ela resolve é que algumas pessoas não precisam de uma lista de todos os sócios da empresa X. Outras querem apenas obter o nome da empresa pelo CNPJ, tornando o retorno de todos os demais dados desnecessários nestes casos. |
Não é só isso. Ela é uma redução de uso de rede em troca de um aumento de uso de processamento no servidor. Normalmente uso de rede é mais barato que custo de processamento do servidor. Por isso sugeri o benchmark já no meu primeiro comentário aqui:
Assim conseguimos quantificar quanto sacrificamos em termos de processamento para diminuir o uso de rede e entendemos melhor o custo desse adicional. Outro ponto relativo ao #167, especialmente no comentário sobre compatibilidade: você teve a chance de verificar como a seleção de campos da resposta funcionaria no pREST para a gente não fazer uma migração sem compatibilidade retroativa? O PR não está gigante em termos de código, acho que o tamanho está super adequado. Estou puxando aqui pontos que acho relevante sobre a contribuição numa conversa que (espero) é produtiva para a gente entender isso melhor. |
Fechado por inatividade. Quando tivermos novidades, aí re-abrimos sem problemas. |
Criei esta feature devido a uma necessidade no trabalho. Agora, é possível selecionar um campo específico na API sem que seja retornado os dados irrelevantes. Reduzindo assim o uso de rede.
É útil em alguns casos, por exemplo se você quer apenas o nome fantasia da empresa.
Veja um exemplo de como obter a unidade federal do cnpj:
É a primeira vez em que eu uso Go, então eu recomendo checar meu código fonte.