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

Fix all i18n issues detected by new script #9499

Conversation

rebecca-shoptaw
Copy link
Collaborator

@rebecca-shoptaw rebecca-shoptaw commented Jun 27, 2024

Closes #9486.

Fix. Manually fixes all i18n syntax issues picked up by the new pre-commit script. Also escapes some non-i18n-able and/or accidentally detected lines.

Technical

  • Involved adjusting the warn regex so that it detects $(' or $(" to avoid creating WARNs for Python code inserts
  • I'm now seeing a SyntaxWarning when the new messages.pot is generated; would be nice to get this fixed before merge, but everything still runs as expected
  • Had to manually escape some Python inserts that began with quotes i.e. $(" " if condition else None) --> $((" " if condition else None)) -- somewhat awkward but only done in ~2-3 infrequently used files, so seems to be worth it
  • There are also two very interesting edge-cases that I left in the EXCLUDE_FILES for now -- /books/edit.html and /history/sources.html -- as both have HTML elements concatenated with +:
    note = '<p class="alert thanks">' + _("We already know about <b>%(work_title)s</b>, but not the <b>%(year)s %(publisher)s edition</b>. Thanks! Do you know any more about this edition?", work_title=work.title, year=edition.publish_date, publisher="; ".join(edition.publishers)) + "</p>"

    I'm not sure the best course of action for handling this would be (i.e. reverting to normal HTML, leaving them in the EXCLUDE_LIST, using the new escape syntax, something else, etc.)

Testing

  1. pre-commit run --all-files with the current much shorter exclude list
  2. You should see several warnings, all intentional to indicate purposefully escaped text, but the test should pass

Screenshot

Stakeholders

@cdrini @pidgezero-one

@rebecca-shoptaw rebecca-shoptaw marked this pull request as ready for review June 27, 2024 15:17
@pidgezero-one
Copy link
Contributor

That's an interesting edge case - just my two cents, if concatenating HTML strings in that manner is infrequent/something we tend to avoid, I think it would be fine to just flag it with $# detect-missing-i18n-skip-line!

@rebecca-shoptaw
Copy link
Collaborator Author

That's an interesting edge case - just my two cents, if concatenating HTML strings in that manner is infrequent/something we tend to avoid, I think it would be fine to just flag it with $# detect-missing-i18n-skip-line!

@pidgezero-one I figure this will probably be the best solution, just a bit awkward for the files themselves as they each have many instances of the concatenated HTML, which will require many flags. But we'll see!

@pidgezero-one
Copy link
Contributor

@rebecca-shoptaw Ahh, I see... It might be a little tricky, but I wonder if it's worth it to add some logic that detects if it's part of a $code block?

I guess HTML within $code blocks might still need i18n, so maybe even just ignoring tags that are followed by ' + _ - but I wonder if this isn't universal enough!

openlibrary/macros/IABook.html Show resolved Hide resolved
Comment on lines +19 to +27
"has_fulltext": _("Ebook"),
"author_key": _("Author"),
"subject_facet": _("Subjects"),
"person_facet": _("People"),
"place_facet": _("Places"),
"time_facet": _("Times"),
"first_publish_year": _("FirstPublished"),
"publisher_facet": _("Publisher"),
"language": _("Language"),
Copy link
Member

Choose a reason for hiding this comment

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

We'll want to confirm that, in cases where we have dictionaries like this, that the values aren't being used as keys elsewhere in the code -- otherwise, once we i18n these keys, we could unintentionally break things. I don't think that's what's happening here, just sharing a possible use case we may hit given this PR touches some 100 files

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Roger that! I'll investigate -- and I believe this is the only dict affected by the PR (the script doesn't actually detect them, I just ran across it).

@rebecca-shoptaw rebecca-shoptaw force-pushed the 9486/fix/fix-i18n-syntax-detected-by-script branch from e7b02c6 to f001b48 Compare June 27, 2024 20:28
@rebecca-shoptaw
Copy link
Collaborator Author

Per @cdrini's advice, closing this to split it up into easier-to-review sub-PRs, which I will list and categorize in the original issue. 🙂

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.

Use new pre-commit script to detect and fix existing i18n syntax issues
3 participants