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

lib: rewrite manifest2po in Python #21532

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

jelly
Copy link
Member

@jelly jelly commented Jan 17, 2025

Extracting translatable strings from manifests is easy enough to do in pure Python and removes an dependency on node_modules.

The differences in the Python implementation versus the JavaScript version:

  • no longer relies on global state to gather the msgid's
  • doesn't encode duplicate filenames for the same msgid

Next up would be porting pkg/lib/html2po to Python, this would allow us to drop html-parser from node_modules.

@jelly jelly added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jan 17, 2025
Copy link
Member

@allisonkarlitskaya allisonkarlitskaya left a comment

Choose a reason for hiding this comment

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

Looks good! I like the structure in general. See the usual bag of small nags.

pkg/lib/manifest2po Outdated Show resolved Hide resolved
pkg/lib/manifest2po Outdated Show resolved Hide resolved
"""


def get_docs_strings(docs: List[Dict[str, str]]) -> Iterable[str]:
Copy link
Member

Choose a reason for hiding this comment

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

This looks more like a Iterable[Mapping[str, str]] but it's not so important...

pkg/lib/manifest2po Outdated Show resolved Hide resolved
yield match


def get_menu_strings(menu: Dict[str, Any]) -> Iterable[str]:
Copy link
Member

Choose a reason for hiding this comment

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

Not crazy about the Any here and the no checking of the actual values of the things you're accessing, but I guess there's room for pragmatism here...

Copy link
Member Author

Choose a reason for hiding this comment

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

So originally I targeted to 3.6, so I didn't have a typeddict, we could make this a typeddict (the original json.load return value). Thoughts?

pkg/lib/manifest2po Outdated Show resolved Hide resolved
pkg/lib/manifest2po Outdated Show resolved Hide resolved
pkg/lib/manifest2po Outdated Show resolved Hide resolved
pkg/lib/manifest2po Outdated Show resolved Hide resolved
pkg/lib/manifest2po Outdated Show resolved Hide resolved
@jelly jelly force-pushed the manifest-po-python branch from 8d8f26d to e3afec2 Compare January 20, 2025 09:54
pkg/lib/manifest2po Outdated Show resolved Hide resolved
pkg/lib/manifest2po Outdated Show resolved Hide resolved
continue

# Qualify the filename if necessary
full_path = args.directory / file if args.directory else file
Copy link
Member

Choose a reason for hiding this comment

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

This strikes me as sort of odd.... are we just doing that because we want to get the correct relative paths in the comments in the output?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose? We do:

mkdir -p po/ && \
./pkg/lib/html2po.js -d . -o po/cockpit.html.pot \
        $(cd . && find pkg/ -name '*.html')

@jelly jelly force-pushed the manifest-po-python branch 2 times, most recently from 1a174a9 to d166d42 Compare January 23, 2025 11:07
@jelly jelly removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jan 24, 2025
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me, and I don't really care about putting that much more effort into this. I took the liberty to close the threads which got fixed. There are some bikeshed-y ones left, not sure how much effort you and @allisonkarlitskaya still want to throw at this.

My one request: Please diff the generated .pot file between main and this PR to make sure it's identical or has only insignificant changes. Thanks!

Copy link
Member

@allisonkarlitskaya allisonkarlitskaya left a comment

Choose a reason for hiding this comment

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

Pretty much agree with Martin here. Let's not spend too too much time here.

Extracting translatable strings from manifests is easy enough to do in
pure Python and removes an dependency on node_modules.

The differences in the Python implementation versus the JavaScript
version:
- no longer relies on global state to gather the msgid's
- doesn't encode duplicate filenames for the same msgid
- no longer replaces @foo@ placeholders which where removed in 473db0e
@jelly jelly force-pushed the manifest-po-python branch from d166d42 to f730d03 Compare January 28, 2025 13:44
@jelly
Copy link
Member Author

jelly commented Jan 28, 2025

Triggered a full CI run as translations do get checked in our integration tests

@martinpitt
Copy link
Member

@allisonkarlitskaya you still have a 👎 finger on here, are you ok with retracting that?

@jelly jelly merged commit 3288a3e into cockpit-project:main Jan 29, 2025
85 checks passed
@jelly jelly deleted the manifest-po-python branch January 29, 2025 10:43
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.

3 participants