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

Added localization framework to mssql-cli #272

Merged
merged 6 commits into from
Aug 30, 2019
Merged

Added localization framework to mssql-cli #272

merged 6 commits into from
Aug 30, 2019

Conversation

geleems
Copy link

@geleems geleems commented Aug 20, 2019

Added localization framework to mssql-cli.

@geleems geleems requested a review from pensivebrian August 20, 2019 23:52
@geleems geleems self-assigned this Aug 20, 2019
Copy link
Member

@pensivebrian pensivebrian left a comment

Choose a reason for hiding this comment

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

Let's chat about this process. It's not clear to me why we need babel - seems a little complicated, so I'd like to understand how you see this process working.

mssqlcli/completion_refresher.py Outdated Show resolved Hide resolved
mssqlcli/mssqlclioptionsparser.py Outdated Show resolved Hide resolved
mssqlcli/locale/mssql-cli.pot Outdated Show resolved Hide resolved
@geleems geleems requested a review from pensivebrian August 21, 2019 18:27
mssqlcli/completion_refresher.py Outdated Show resolved Hide resolved
mssqlcli/completion_refresher.py Outdated Show resolved Hide resolved
@geleems geleems force-pushed the gelee/loc branch 2 times, most recently from c278bc4 to fe9633f Compare August 21, 2019 19:18
@geleems geleems requested a review from pensivebrian August 21, 2019 19:49
mssqlcli/i18n.py Outdated Show resolved Hide resolved
mssqlcli/i18n.py Outdated Show resolved Hide resolved
@pensivebrian
Copy link
Member

The are some strings that need to be translated in main.py

@pensivebrian
Copy link
Member

Same with mssql_cli.py. Are you planning on making a complete loc string pass in another PR?

mssqlcli/main.py Outdated Show resolved Hide resolved
@pensivebrian
Copy link
Member

Is it possible to add a unit test to make sure all of this works?

@geleems
Copy link
Author

geleems commented Aug 28, 2019

Unit test for testing localization is added.

@geleems geleems requested a review from pensivebrian August 28, 2019 00:57
@pensivebrian pensivebrian self-requested a review August 28, 2019 15:33
Copy link
Member

@pensivebrian pensivebrian left a comment

Choose a reason for hiding this comment

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

Heading in the right direction.

What do you think about moving all localized strings to a type LocalizedString like we do for resource files on Windows?

tests/test_localization.py Outdated Show resolved Hide resolved
@geleems geleems requested a review from pensivebrian August 29, 2019 02:39
mssqlcli/i18n.py Outdated Show resolved Hide resolved
mssqlcli/jsonrpc/contracts/connectionservice.py Outdated Show resolved Hide resolved
mssqlcli/mssqlclioptionsparser.py Outdated Show resolved Hide resolved
tests/test_localization.py Outdated Show resolved Hide resolved
@geleems geleems requested a review from pensivebrian August 29, 2019 23:11
build.py Outdated Show resolved Hide resolved
mssqlcli/localized_strings.py Outdated Show resolved Hide resolved
mssqlcli/mssql_cli.py Outdated Show resolved Hide resolved
mssqlcli/mssqlclioptionsparser.py Outdated Show resolved Hide resolved
mssqlcli/localized_strings.py Show resolved Hide resolved
tests/test_localization.py Outdated Show resolved Hide resolved
Copy link
Member

@pensivebrian pensivebrian left a comment

Choose a reason for hiding this comment

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

Approved with some nit issues to address before checkin.

@@ -120,8 +120,7 @@ def create_parser():
dest=u'multi_subnet_failover',
action=u'store_true',
default=False,
help=u'If application is connecting to AlwaysOn AG on different subnets, setting this provides faster '
u'detection and connection to currently active server.')
help=u'If application is connecting to AlwaysOn AG on different subnets, setting this provides faster detection and connection to currently active server.')
Copy link
Member

Choose a reason for hiding this comment

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

Let's revert this change as it's not related to loc.


## Localized Strings
def goodbye():
return _(u'Goodbye!')
Copy link
Member

Choose a reason for hiding this comment

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

Add a newline?

@@ -169,6 +171,44 @@ def validate_actions(user_actions, valid_targets):
sys.exit(1)


def generate_mo(extraction_target_path, lang_name, trans_mappings, domain, localedir=None):
Copy link
Member

Choose a reason for hiding this comment

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

We should separate out the po and mo generation, but we can do this in another PR.

@geleems geleems merged commit 1cfcf5f into master Aug 30, 2019
@geleems geleems deleted the gelee/loc branch September 18, 2019 21:45
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.

2 participants