okfn-brasil / querido-diario

📰 Diários oficiais brasileiros acessíveis a todos | 📰 Brazilian government gazettes, accessible to everyone.
https://queridodiario.ok.org.br/
MIT License
1.08k stars 392 forks source link

[Discussão] Revisão automática de raspadores #1271

Open trevineju opened 17 hours ago

trevineju commented 17 hours ago

Tenho pensado que poderíamos ter uma etapa de revisão básica e automática no repositório, para ser engatilhada cada vez que uma nova PR chega. Como o código do QD é muito padronizado, daria para ter uma lista de critérios assim:

  1. Verificação de consistência de nomenclatura Garantir que nome do arquivo (uf_municipio.py), nome da classe (UFMunicipioSpider) e nome da spider (name = "uf_nome_do_municipio") sejam correspondentes.

    O nome do arquivo e o nome da spider não é comum de errarem (até pq precisa pra executar a spider), mas o nome da classe é comum ficar diferente na hora de copiar do modelo ou do código de outros arquivos.

  2. Verificação do TERRITORY_ID Garantir que é o TERRITORY_ID certo, buscando em territories.csv a ocorrência do ID e vendo se é o município-UF corresponde ao name no raspador.

    Particularmente chato quando o município é homônimo com algum outro.

  3. Verificação de domínio Garantir que seja só o domínio mesmo, sem http/https, www, ou coisas depois de .br/

  4. Coerência da data Se a linha start_date = date(AAAA, MM, DD) tem valores no intervalo certo: DD [1:31], MM [1:12] e AAAA maior que 1800 e menor que "hoje".

    Essa aqui parece estranha, mas fora a parte de em ptbr ser em outra ordem, já tive caso disso por conta do #919 - foi typo no ano, era pra ser 2008, mas no site da prefeitura tava 2080, algo assim. Começar a pegar coisa automaticamente em site fica refém dessas coisas e poderiamos dar uma blindada.

  5. Nome certo para as variáveis Se está escrito custom_settings e não custom_setting.

    Já aconteceu e isso não dá erro com o Scrapy visto que a variável fica existindo ali no escopo da classe como um dicionário qualquer.

E continua...

Porém, esses componentes não aparecem em todo raspador. Mas como temos apenas 3 tipos de raspadores (raspador individual, spider base e um raspador herdeiro de spider base) e isso está bem consolidado já, pode ser possível criar verificações para cada um.

Teria que continuar essa tabela uma vez que uma lista de critérios seja elaborada, mas algo nessa linha: tipo Nomenclatura TERRITORY_ID Domínio Data
individual sim sim sim sim
base sim, entre nome do arquivo e nome da classe não sim não
herdeiro sim sim sim sim

Uma rotina desse tipo não supre a demanda de revisão, mas poderia focar nesses detalhes de padrão de código que, às vezes, passa até no olhar humano. Esse tipo de inconsistência "boba" é coisa que já tivemos que corrigir entre contribuidores, até mesmo experientes. Eu já deixei coisa assim passar em PR que chega e tem 10 ou 15 raspadores herdeiros de uma vez -- o olho vicia por eles serem muitos e praticamente iguais.

Também acho que poupa os revisores de terem que pedir esse grau de correção. Uma pessoa contribuidora já receberia um feedback de ajustes mais básicos na PR logo de cara.

O que acham?

trevineju commented 10 hours ago

em #463 também tem outras ideias levantadas e em #464 até um começo de implementação

ayharano commented 8 hours ago

Batendo o olho, parece que os itens 2, 3 e 5 talvez sejam resolvíveis por um ou mais unittest.TestCase (isso para usar a stdlib sem precisar instalar pytest por exemplo) em cima de todos os spiders no estado do commit

Como está a questão de pipeline dos raspadores no momento? Pergunto se vale a pena incluir um TestCase e sempre rodar a cada commit

Não sei se automatizaria muito, mas podemos validar atributos de classes e cruzar info do CSV, etc

jjpaulo2 commented 5 hours ago

Gostei bastante da ideia! Parabéns @trevineju pela iniciativa.

Também gostei da ideia que o @ayharano trouxe. Acho que daria pra fazer o teste gerar alguma saída, e a gente coloca uma Action pra ler essa saída e gerar um comentário no PR.