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

Github Action para ejecutar powrap fix con un comentario #1836

Open
wants to merge 34 commits into
base: 3.10
Choose a base branch
from

Conversation

erickisos
Copy link
Contributor

@erickisos erickisos commented Oct 5, 2022

Descripción

Esta es la versión probada de la Github Action para ejecutar pospell con un comentario.

Uso

Los pasos del workflow son los siguientes:

  • Comenta en un PR pospell-fix
  • El Github Bot reaccionará con un 👍 a tu comentario, para notificarte que se está ejecutando.
  • Si tras ejecutar powrap y pospell el bot encuentra cambios, se generará un commit directamente a tu PR.

Referencias

@erickisos erickisos changed the title Pospell-fix Github Action Github Action para ejecutar pospell fix con un comentario Oct 5, 2022
Este cambio no está relacionado al PR, así que no hace sentido tenerlo acá.
@erickisos
Copy link
Contributor Author

Acá está una versión que saqué después de probar en mi fork @rtobar @ezio-melotti 🥳

@rtobar
Copy link
Collaborator

rtobar commented Oct 5, 2022

pospell-fix

run: python -m pip install -r requirements.txt
- name: Ejecutar Powrap
run: powrap --quiet **/*.po
- name: Revisar con Pospell
Copy link
Collaborator

Choose a reason for hiding this comment

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

Como mencionaba en el otro PR, pospell no hace ningún cambio local en tus archivos, sólo chequea si la ortografía es correcta o no, y retorna con status 0 o distinto a 0 dependiendo de si encontró errores. Esto significa que hacer chequeo ortográfico no tiene incidencia alguna en el paso siguiente (hacer un commit con los cambios locales), por lo que encuentro que no tiene mucho sentido ejecutar pospell en primer lugar -- aparte que hace que la acción se demore más en terminar.

En mi opinión la acción debería estar centrada solo en ejecutar powrap y hacer el commit con los cambios que este encuentre, nada más (al menos esa era mi intención al crear #1786). Por eso también insistía en que los nombres del archivo, de la acción, etc, estuvieran todos centrados en powrap, no en pospell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh vaya, got it, déjame hacer esos cambios de una, la verdad no me había quedado claro 😅

scripts/check_spell.py Outdated Show resolved Hide resolved
Copy link
Member

@ezio-melotti ezio-melotti left a comment

Choose a reason for hiding this comment

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

Algunas sugerencias sobre un nuevo comando pospell-check que solo dice si la PR es buena sin hacer el commit.
Tambien podrias ejecutar el check automaticamente cuando la PR es creada/modificada, y solo haber el comando pospell-fix para hacer el commit.

run: python scripts/check_spell.py
continue-on-error: true
- name: Commit & Push changes
uses: actions-js/push@master
Copy link
Member

Choose a reason for hiding this comment

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

Tambien hay esta action que parece hacer el mismo, pero en el marketplace tiene 1k star (actions-js/push solo tiene 38).
Nunca las he probado, así que no puedo decirte cuál es la mejor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hice el cambio solo para hacer la prueba, si vemos que no nos funciona as expected, le damos de vuelta a la que teníamos

.github/workflows/pospell-on-demand.yml Outdated Show resolved Hide resolved
.github/workflows/pospell-on-demand.yml Outdated Show resolved Hide resolved
.github/workflows/pospell-on-demand.yml Outdated Show resolved Hide resolved
@erickisos
Copy link
Contributor Author

powrap-fix

@erickisos erickisos requested review from rtobar and ezio-melotti and removed request for rtobar October 5, 2022 04:40
@erickisos
Copy link
Contributor Author

Ya hice varios de los cambios sugeridos, pero lamentablemente creo que no podemos ejecutarlo hasta que le demos merge en este repo 😞

De todas maneras por si las moscas, voy a crear un PR en mi fork, para probar la ejecución.
@rtobar

@erickisos erickisos requested review from rtobar and removed request for ezio-melotti October 5, 2022 04:44
@erickisos
Copy link
Contributor Author

Tras los cambios, en este PR (erickisos#6) ejecuté el comando y podemos ver los resultados acá

@erickisos
Copy link
Contributor Author

Desafortunadamente me he dado cuenta de que el commit action no encuentra cambios tras ejecutar powrap, no estoy seguro de por qué 😢

@erickisos erickisos changed the title Github Action para ejecutar pospell fix con un comentario Github Action para ejecutar powrap fix con un comentario Oct 8, 2022
@cmaureir cmaureir added the hacktoberfest-accepted mantainer-approved contribution to hacktoberfest label Oct 25, 2022
Copy link
Collaborator

@rtobar rtobar left a comment

Choose a reason for hiding this comment

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

Perdón @erickisos por tomarme tanto tiempo en responder pero andaba de vacaciones y sin mucho tiempo para ver otros temas.

Veo que la mayoría de los comentarios ya los tomaste en cuenta, y la acción ahora sólo ejecuta powrap como era la intención original. Estuve viendo los ejemplos de ejecución que pusiste, y otros más, y encontré otro par de problemas que hay que arreglar, pero en general ser está viendo súper bien 👍

.github/workflows/execute-powrap.yml Outdated Show resolved Hide resolved
.github/workflows/execute-powrap.yml Outdated Show resolved Hide resolved
.github/workflows/execute-powrap.yml Outdated Show resolved Hide resolved
.github/workflows/pospell-on-demand.yml Outdated Show resolved Hide resolved
.github/workflows/execute-powrap.yml Show resolved Hide resolved
@humitos
Copy link
Collaborator

humitos commented Oct 28, 2022

En #1786 (comment) comenté porqué creo que esto puede ser un problema, y propuse una solución "inofensiva" a primera vista.

@erickisos
Copy link
Contributor Author

Hey, hola a todos, le di ya una revisada a los comments, pero me parece super interesante el que menciona @humitos por acá (#1786), Qué opinan ustedes?

@rtobar @ezio-melotti

@github-actions
Copy link

Este PR lleva un tiempo sin actualizaciones. Vamos a pedir a un admin de nuestro equipo que decida si alguien más puede finalizarlo o si tenemos que cerrarlo.\nPor favor, avisanos en caso de que aún puedas terminarlo.

@erickisos
Copy link
Contributor Author

Estaría bueno revisar si podemos etiquetar gente con el stale bot, supongo que después

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted mantainer-approved contribution to hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants