Skip to content

Fix create issue script#2876

Merged
cacrespo merged 9 commits into
python:3.12from
sofide:fix_create_issue_script
Nov 21, 2024
Merged

Fix create issue script#2876
cacrespo merged 9 commits into
python:3.12from
sofide:fix_create_issue_script

Conversation

@sofide

@sofide sofide commented Nov 21, 2024

Copy link
Copy Markdown
Collaborator

Algunas mejoras para el script que genera issues.

Lo probé en mi fork y me generó por ejemplo este issue: sofide#2

Comment thread scripts/create_issue.py Outdated


def check_issue_not_already_existing(pofilename):
issues = repo.get_issues(state='open')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Supongo que no es un problema, pero sería bueno revisar cómo cachar esta request, veo que nos traemos todos los issues y supongo esto ocurre para cada pofile.

@rtobar rtobar Nov 21, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Totalmente de acuerdo con el comentario, no creo que sea mucho cambiar el código para hacer una sola query?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done por acá: 6916e38

@rtobar rtobar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Súper contribución @sofide! Dejo algunos comentarios, pero nada urgente, tómalos o déjalos

Comment thread scripts/create_issue.py Outdated
repo = g.get_repo('python/python-docs-es')

PYTHON_VERSION = "3.13"
ISSUE_LABELS = [PYTHON_VERSION, "good first issue"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

El "good first issue" yo lo aplicaría sólo para issues donde jay que traducir menos de N entradas (5, 10?) en vez de aplicarlo de forma indiscriminada a todos

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Me gustó la idea, done acá: 1915095

Comment thread scripts/create_issue.py Outdated
answer = input(msg)
if answer != 'y':
sys.exit(1)
- Fuzzy: {pofile_fuzzy}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

La forma en que estas estadísticas se muestran siempre me ha parecido un poco confusa, porque hay que hacer un poco de matemáticas para entender cuántas entradas de verdad necesitan trabajo. Parte de esa confusión (creo) también es que las entradas fuzzy creo que cuentan para el pofile.percent_translated, pero puedo estar equivocando (o quizás ha cambiado eso en potodo).

Yo mostraría la información más o menso así (usando formato de f-string para ejemplificar nada más)

- Total entries: {T}
- Entries that need work: {F + U} ({(F + U)/T * 100:.2f} %)
  - Fuzzy: {F}
  - Untranslated: {U}

Dime qué te parece la idea.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

De acuerdo en que son confusas.
Mejorado acá 1915095 y luego arreglado acá 9938321

Comment thread scripts/create_issue.py
if len(sys.argv) != 2:
raise Exception(error_msg)

arg = sys.argv[1]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Una idea que tenía era agregar un modo "dry run", pero queda para el futuro, necesitaría más cambios de los que hay hasta el momento

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

de acuerdo con la idea, y con que se haga a futuro 😅

Comment thread scripts/create_issue.py Outdated
if pofilename in issue.title:

print(f'Skipping {pofilename}. There is a similar issue already created at {issue.html_url}')
raise IssueAlreadyExistingError()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nitpick: no hace falta instanciar acá

Suggested change
raise IssueAlreadyExistingError()
raise IssueAlreadyExistingError

Comment thread scripts/create_issue.py Outdated
pofile.untranslated == 0,
]):
print(f'Skipping {pofile.filename}. The file is 100% translated already.')
raise PoFileAlreadyTranslated()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ni acá

Comment thread scripts/create_issue.py Outdated


def check_translation_is_pending(pofile):
if pofile.fuzzy == 0 and any([

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Veo que este any estaba en el original, pero lo encuentro innecesariamente confuso. En vez de hacer any([a, b]) puedes hacer (a or b).

Pero creo que la lógica podría ser simplificada de todas formas. Se me hace que if not pofile.fuzzy and not pofile.untranslated sería suficiente, pero habría que probar

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

De acuerdo, mejoré un poco esto acá: 9461867

@sofide

sofide commented Nov 21, 2024

Copy link
Copy Markdown
Collaborator Author

Con los últimos cambios volví a correrlo en mi fork personal y se creó este issue, para que vean de ejemplo: sofide#7

@cacrespo cacrespo merged commit 30a3564 into python:3.12 Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants