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.

Sign up for GitHub

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

Jump to bottom

Experiments with gettext #328

Merged
merged 1 commit into from
Dec 10, 2021
Merged

Experiments with gettext #328

merged 1 commit into from
Dec 10, 2021

Conversation

mdellweg
Copy link
Member

@mdellweg mdellweg commented Aug 4, 2021

No description provided.

@mdellweg mdellweg force-pushed the i18n_experiment branch 3 times, most recently from 297b10d to 8107fe4 Compare August 9, 2021 14:35
@mdellweg mdellweg marked this pull request as ready for review October 21, 2021 11:17
@ggainey
Copy link
Contributor

ggainey commented Nov 2, 2021

Remind me what we were waiting for on this PR? I've tested what you have here, and it works as I'd expect it to. Ready for review, or do have more work to do?

@mdellweg
Copy link
Member Author

mdellweg commented Nov 3, 2021 •
edited
Loading

Now that you ask, we should probably see if we can extend this model to external plugins.

Edit: I believe this is now possible:
pulp/pulp-cli-deb#10

@mdellweg
Copy link
Member Author

mdellweg commented Nov 3, 2021 •
edited
Loading

Another question is whether we can deliver the translations with the python package. Not sure how to test this before actually releasing.

Edit: I am sufficiently confident, the translation files (*.mo) are now part of the distribution. Also having the translations in the plugin directory should help to move them around in the future.

@mdellweg mdellweg force-pushed the i18n_experiment branch 7 times, most recently from 691a537 to 2a5196c Compare November 7, 2021 11:16
@mdellweg
Copy link
Member Author

mdellweg commented Nov 7, 2021

@ggainey This is ready for review now.

gerrod3
gerrod3 reviewed Dec 2, 2021
View reviewed changes
Copy link
Contributor

@gerrod3 gerrod3 left a comment

Choose a reason for hiding this comment

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

Most of the translation files seem kind of empty. What is the process for translating the strings of the CLI?

Makefile
@@ -1,3 +1,10 @@
LANGUAGES=de
Copy link
Contributor

Choose a reason for hiding this comment

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

We only adding German support for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

We would add them as we go.

pulpcore/cli/ansible/locale/de/LC_MESSAGES/messages.po Outdated
Comment on lines 1 to 5
# SOME DESCRIPTIVE TITLE.
# Copyright (C) YEAR THE PACKAGE'S COPYRIGHT HOLDER
# This file is distributed under the same license as the PACKAGE package.
# FIRST AUTHOR <EMAIL@ADDRESS>, YEAR.
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be filled out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably

setup.py
Comment on lines +7 to +9
plugin_packages = find_namespace_packages(
include=["pulpcore.cli.*"], exclude=["pulpcore.cli.*.*"]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this method think the translation files are packages?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was listing all submodules too.

setup.py
@@ -41,7 +42,7 @@
url="https://github.com/pulp/pulp-cli",
version="0.13.0.dev",
packages=plugin_packages + extra_packages,
package_data={package: ["py.typed"] for package in plugin_packages},
package_data={"": ["py.typed", "locale/*/LC_MESSAGES/*.mo"]},
Copy link
Contributor

Choose a reason for hiding this comment

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

For loop no longer needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was never needed. Setuptools can handle these wildcard style.

@mdellweg
Copy link
Member Author

mdellweg commented Dec 6, 2021

It's that time of the Year again, where different versions of black play ping pong with your code. 🏓 Fun!

@ggainey
Copy link
Contributor

ggainey commented Dec 6, 2021

It's that time of the Year again, where different versions of black play ping pong with your code. ping_pong Fun!

Fun! indeed - the PR needs a rebase anyway, will it get Magically Fixed once you do that?

@mdellweg mdellweg added this to the 0.13.0 milestone Dec 9, 2021
ggainey
ggainey approved these changes Dec 10, 2021
View reviewed changes
Copy link
Contributor

@ggainey ggainey left a comment

Choose a reason for hiding this comment

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

Great start - gives us the foundation pieces we need to build on!

@ggainey ggainey merged commit 8bfdc2a into pulp:main Dec 10, 2021
@mdellweg mdellweg deleted the i18n_experiment branch December 10, 2021 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gerrod3 gerrod3 gerrod3 left review comments

@ggainey ggainey ggainey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.
3 participants
@mdellweg @ggainey @gerrod3