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

Remove some untranslatable characters from msgids #307

Merged
8 commits merged into from Mar 2, 2023
Merged

Remove some untranslatable characters from msgids #307

8 commits merged into from Mar 2, 2023

Conversation

ghost
Copy link

@ghost ghost commented Feb 15, 2023

Purpose

Improve msgids consistency by removing unnecessary parts.

Context

Partially addresses #194

Left for an upcoming PR:

  • removing trailing newlines from msgids
  • naming arguments in msgid
  • remove any tabulator space

Changes

  • Remove trailing spaces from msgid
  • Remove untranslatable msgid (the one composed of the "=" char)
  • Make the share directory accessible in development environment

How to test this PR

Zonemaster-CLI behavior should not be altered. Running zonemaster-cli --show-testcase --show-module --locale <locale> <domain> with different locale should not alter the header output.

@ghost ghost added this to the v2023.1 milestone Feb 15, 2023
@ghost ghost requested review from matsduf, hannaeko, tgreenx and marc-vanderwal February 15, 2023 12:05
Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

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

It is great that you take this first step. Do you plan to create further PRs for the remaining of issue #194?

I have not yet tested, but will do that.

Could you do the same technical updates of the other PO files to relieve the translator from that?

All PO files are planned to be updated in v2022.2.2 so I suggest that this PR is merged after the release to let this PR handle any conflicts. Is that OK?

@ghost
Copy link
Author

ghost commented Feb 15, 2023

Do you plan to create further PRs for the remaining of issue #194?

This is the idea. Or someone else could do it. I don't really mind. As long as we're moving forward on this.

Could you do the same technical updates of the other PO files to relieve the translator from that?

Will do. Done.

All PO files are planned to be updated in v2022.2.2 so I suggest that this PR is merged after the release to let this PR handle any conflicts. Is that OK?

This is totally fine for me (even though I don't expect any conflict with the updated msgid).

@ghost ghost requested a review from matsduf February 15, 2023 12:58
@matsduf
Copy link
Contributor

matsduf commented Feb 15, 2023

Do you plan to create further PRs for the remaining of issue #194?

This is the idea. Or someone else could do it. I don't really mind. As long as we're moving forward on this.

I agree. I had this update in my mind.

Could you do the same technical updates of the other PO files to relieve the translator from that?

Will do. Done.

Thanks.

All PO files are planned to be updated in v2022.2.2 so I suggest that this PR is merged after the release to let this PR handle any conflicts. Is that OK?

This is totally fine for me (even though I don't expect any conflict with the updated msgid).

Maybe not in msgid, but this PR adds empty msgstr that #303 adds as non-empty strings. And this PR updates POT-Creation-Date which is updated by #303 to another value.

@matsduf matsduf dismissed their stale review February 15, 2023 13:11

All PO files are updated.

@@ -397,19 +419,19 @@ sub run {
if ( $translator ) {
my $header = q{};
if ( $self->time ) {
$header .= sprintf "%7.2f ", $entry->timestamp;
$header .= sprintf "%${field_width{seconds}}.2f ", $entry->timestamp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a remark on style: you can have dynamic field widths as arguments to sprintf. These calls to sprintf could then be rewritten as:

sprintf "%*.2f ", field_width(seconds), $entry->timestamp
sprintf "%-*s ", field_width(level), translate_severity($entry->level);
sprintf "%*.2f %-*s ", field_width(seconds), $entry->timestamp, field_width(level), $entry->level;

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the tip. I've updated the code.

tgreenx
tgreenx previously approved these changes Feb 22, 2023
Copy link
Contributor

@tgreenx tgreenx left a comment

Choose a reason for hiding this comment

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

LGTM, tested and works as described.

marc-vanderwal
marc-vanderwal previously approved these changes Feb 23, 2023
Copy link
Contributor

@marc-vanderwal marc-vanderwal 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 to me too.

@matsduf
Copy link
Contributor

matsduf commented Feb 23, 2023

This is fine, but should not be merged until v2022.2.2 has been released and its branch merged into develop branch.

@ghost ghost dismissed stale reviews from marc-vanderwal and tgreenx via 4c184ea March 2, 2023 10:17
@ghost
Copy link
Author

ghost commented Mar 2, 2023

This is fine, but should not be merged until v2022.2.2 has been released and its branch merged into develop branch.

Rebased and removed conflict with develop. Please re-review.

@ghost ghost merged commit ba17e20 into zonemaster:develop Mar 2, 2023
@ghost ghost deleted the rm-untranslatable branch March 13, 2023 14:22
@mattias-p
Copy link
Member

I found and created #334 when release testing of this PR.

@ghost ghost mentioned this pull request May 30, 2023
@mattias-p
Copy link
Member

I've successfully tested this as part of release testing for 2023.1.

@mattias-p mattias-p added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jun 5, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-ReleaseTested Status: The PR has been successfully tested in release testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants