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

[trevineju]

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 Mapeamento automatizado de cidades #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]

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

[ayharano]

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]

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.

[anapaulagomes]

Seria uma mão na roda mesmo ter algo assim! Acho que instalar o pytest não seria um grande problema, visto que existem benefícios que tanto a facilidade escrever os testes quanto o ecossistema de plugins traria. Vale até checar se já não existe algum plugin pro scrapy.

Sobre os padrões de nome ou coisas no código, tem o https://github.com/codingjoe/relint. Dá pra montar um linter criando algumas das regras que você descreveu em regex.

[ogecece]

Acho ótimo o caminho do teste unitário (também não vejo problema em adicionar pytest como dependência) e relint pros padrões de código do projeto e poderia ser executada em cada commit sem problema (preferencialmente em pre-commit pra nem subir com erro e também já temos github action configurada pra testar usando os hooks do pre-commit).

Já a ideia trazida em #463 acho que é bem diferente pq não faria muito sentido aplicar no projeto todo de uma vez nem executar raspadores incompletos após cada commit. Acho bem legal o caminho que a @anapaulagomes tinha trazido, de ser um comando para ser executado. E se pudesse ser executado como Github action sob demanda por pessoas mantenedoras fazendo um report como o @jjpaulo2 falou seria melhor ainda.

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

@ayharano qual questão de pipeline você diz?

[ayharano]

@ayharano qual questão de pipeline você diz?

Era mais para ver se iria executar junto com o pre-commit ou como uma outra chamada a parte dentro do workflow de tests

https://github.com/okfn-brasil/querido-diario/blob/main/.github%2Fworkflows%2Ftests.yml

[ogecece]

Ah, saquei! Acho que esse tipo de verificação deveria estar no pre-commit mesmo. O tempo de verificação do commit seria negligente comparado com a dor de cabeça que é ficar revisando essas minúcias

[trevineju]

Concordo que poderia ser verificado desde o pre-commit, e as atuais actions do repositório já testam essa rotina entregando o log na opção "details" da PR. Isso já seria suficiente? Ou acham que precisa de mais destaque e ser uma msg na PR mesmo?

Outra coisa, pra viabilizar isso precisa do quê? criar um arquivo (ou diretório) com a lógica da verificação e atualizar o .pre-commit-config.yaml para usá-lo?

[ogecece]

Concordo que poderia ser verificado desde o pre-commit, e as atuais actions do repositório já testam essa rotina entregando o log na opção "details" da PR. Isso já seria suficiente? Ou acham que precisa de mais destaque e ser uma msg na PR mesmo?

Na minha opinião já seria suficiente

Outra coisa, pra viabilizar isso precisa do quê? criar um arquivo (ou diretório) com a lógica da verificação e atualizar o .pre-commit-config.yaml para usá-lo?

Pelo que entendi, seria isso mesmo. Criar um diretório de testes e um arquivo .relint.yml. E no pre-commit dá pra configurar hooks a partir do repo local e executar comando de CLI, inclusive com dependências python