-
Notifications
You must be signed in to change notification settings - Fork 385
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
Fix create issue script #2876
Fix create issue script #2876
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á
raise IssueAlreadyExistingError() | |
raise IssueAlreadyExistingError |
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 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
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 |
Algunas mejoras para el script que genera issues.
Lo probé en mi fork y me generó por ejemplo este issue: sofide#2