- Notifications
You must be signed in to change notification settings - Fork397
Fix create issue script#2876
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
scripts/create_issue.py Outdated
| def check_issue_not_already_existing(pofilename): | ||
| issues = repo.get_issues(state='open') |
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.
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.
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.
Totalmente de acuerdo con el comentario, no creo que sea mucho cambiar el código para hacer una sola query?
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.
Done por acá:6916e38
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.
Súper contribución@sofide! Dejo algunos comentarios, pero nada urgente, tómalos o déjalos
scripts/create_issue.py Outdated
| repo = g.get_repo('python/python-docs-es') | ||
| PYTHON_VERSION = "3.13" | ||
| ISSUE_LABELS = [PYTHON_VERSION, "good first issue"] |
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.
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
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 gustó la idea, done acá:1915095
scripts/create_issue.py Outdated
| answer = input(msg) | ||
| if answer != 'y': | ||
| sys.exit(1) | ||
| - Fuzzy: {pofile_fuzzy} |
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.
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.
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.
| if len(sys.argv) != 2: | ||
| raise Exception(error_msg) | ||
| arg = sys.argv[1] |
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.
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
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.
de acuerdo con la idea, y con que se haga a futuro 😅
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() |
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.
Nitpick: no hace falta instanciar acá
| raiseIssueAlreadyExistingError() | |
| raiseIssueAlreadyExistingError |
scripts/create_issue.py Outdated
| pofile.untranslated == 0, | ||
| ]): | ||
| print(f'Skipping {pofile.filename}. The file is 100% translated already.') | ||
| raise PoFileAlreadyTranslated() |
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.
Ni acá
scripts/create_issue.py Outdated
| def check_translation_is_pending(pofile): | ||
| if pofile.fuzzy == 0 and any([ |
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.
Veo que esteany estaba en el original, pero lo encuentro innecesariamente confuso. En vez de hacerany([a, b]) puedes hacer(a or b).
Pero creo que la lógica podría ser simplificada de todas formas. Se me hace queif not pofile.fuzzy and not pofile.untranslated sería suficiente, pero habría que probar
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.
De acuerdo, mejoré un poco esto acá:9461867
Con los últimos cambios volví a correrlo en mi fork personal y se creó este issue, para que vean de ejemplo:sofide#7 |
30a3564 intopython:3.12Uh oh!
There was an error while loading.Please reload this page.
Algunas mejoras para el script que genera issues.
Lo probé en mi fork y me generó por ejemplo este issue:sofide#2