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

Add Format Markers #152

Merged
merged 15 commits into from
May 6, 2020
Merged

Add Format Markers #152

merged 15 commits into from
May 6, 2020

Conversation

shundhammer
Copy link
Contributor

@shundhammer shundhammer commented Apr 30, 2020

Trello

https://trello.com/c/Z4PNrMjq/

Bugzilla

https://bugzilla.suse.com/show_bug.cgi?id=954505
https://bugzilla.suse.com/show_bug.cgi?id=980329

Problem

Our translators have a hard time to always get the placeholders right in all the many messages in YaST, and we have several different types of them:

  • printf()-like: %s, %d, ... (and much more complex with field widths etc.)
  • sformat()-like: %1, %2, ...

They use tools that are mostly beyond our control; Weblate or even some Windows programs.

First (Failed) Attempt

Extend the GNU gettext tools to get native support for Ruby in them.

This turned out to be a major task with a hand-crafted Ruby parser for the GNU gettext tools. Ruby is hard to parse, and the available tools in that GNU gettext environment are very basic: Just plain C with the ususal poor C string handling and no regexps etc.; this was abandoned. It would have been a pretty simplistic and probably bug-ridden parser. It is doubtful if that would even have been accepted upstream.

Chosen Pragmatic Approach

Rather than extend the GNU gettext tools and hope for the changes to be accepted and waiting for them to become released and distributed to the translators, this approach uses available mechanisms:

GNU gettext already supports YCP as well as C (and a number of other programming languages, but not Ruby).

You can add markers in the source code to give gettext a hint what format a message is in, and then it will be checked by tools like msgformat and msgmerge. Available translation tools tend to use that to do at least some amount of error checking.

# xgettext:ycp-format
_("Foo %1 bar %2")

.pot file generated by xgettext (regardless of detected programming language):

#, ycp-format
msgid "Foo%1 bar %2"
msgstr ""

Rather than relying on the extraction tools to get this right automatically or adding yet another translation tools related comment manually to the YaST source code, this pull request introduces a new tool to detect our known formats and add that marker to the .pot files after extracting the messages with rxgettext (the Ruby variant of xgettext from the gettext Ruby gem).

This is intended to be called from y2makepot which is called by rake potfiles.

Usage

Usage: po_add_format_hints [OPTIONS] PO_FILE1 [PO_FILE2] ...

Add PO format marker hints to .pot and .po files:
  "%s %d" etc.         -> c-format
  "%1 %2" etc.         -> ycp-format
  "%{foo} %{bar}" etc. -> perl-brace-format

The markers are added in-place unless the --dry-run option is given.

Use the usual xgettext comments to override this:
  # xgettext:c-format
  foo(_("... %1 ..."))

Options:
    -n, --dry-run                    Dry run; don't modify the file.
    -p, --no-perl-brace-format       Don't use the perl-brace-format
    -v, --verbose                    More output
    -s, --silent                     No output

Supported Formats

  • Simple printf-like: %s, %d, %x, ...
    -> c-format

  • Complex printf-like: %02x, %10.4d, %-10.4d, ...
    -> c-format

  • printf-like with positional parameters: %1$d, %2$s, ...
    -> c-format

  • sformat-like with positional parameters: %1, %2, ...
    -> ycp-format

    Notice that c-format has precedence, so %2d etc. has precedence over %2.

  • named parameters: %{foo}, %{bar}
    -> perl-brace-format

    Caveat: perl-brace-format is really only the part without the %, so msgfmt -c will only check the {foo} or {bar} part, not the complete %{foo} or %{bar}. So there is still some potential for translators to get this wrong, but it's clearly much better than not checking those named parameters at all.

    If there are problems, this format can be disabled with the --no-perl-brace-format (or -p) command line switch.

Unsupported Format

  • Named parameters with a format spec: %<foo>s, %<bar>02x

    We only have a handful (2-5) of those all over YaST, and AFAICS never any more complex than %<foo>s or %<bar>d, so they can easily be replaced by their simpler equivalent %{foo} or %{bar}.

How to Use It

Simply call rake potfiles or y2makepot. Of course you can also call it directly.

Preferably, call this on the .pot files before merging them with the last translations, so the format markers are propagated into all the individual .po files for each language.

It is also possible, of course, to call this on all the .po files, if that is desired.

This script changes the files in-place, i.e. it writes the changes back to each file directly, i.e.

po_add_format_hints myfile1.pot myfile2.pot

modifies myfile1.pot and myfile2.pot.

Invoke it with

po_add_format_hints --dry-run --verbose myfile1.pot myfile2.pot

or

po_add_format_hints -n -v myfile1.pot myfile2.pot

to see what it would do. Or simply let it modify the files and check with git diff what it changed.

Test

Sample Output

rake pot in yast-storage-ng (with --verbose):

/usr/bin/y2tool y2makepot
Creating ./storage.pot from  \
./src/lib/y2partitioner/actions/add_bcache.rb \
./src/lib/y2partitioner/actions/add_btrfs.rb \
./src/lib/y2partitioner/actions/add_lvm_lv.rb \
./src/lib/y2partitioner/actions/add_lvm_vg.rb \
...
...
Parsing storage.pot (871 entries)
  Detected simple printf()-like format
* Adding c-format to "No free space left in the volume group "%s"."
  Detected named parameters %{value}
* Adding perl-brace-format to "The device %{name} is directly formatted.\nRemove ..."
  Detected named parameters %{value}
* Adding perl-brace-format to "It is not possible to create a partition on %{name..."
  Detected simple printf()-like format
* Adding c-format to "Add Partition on %s"
  Detected simple printf()-like format
* Adding c-format to "Encryption for %s"
  Detected simple printf()-like format
* Adding c-format to "Modify encryption of %s"
  Detected simple printf()-like format
* Adding c-format to "Encrypt %s"
  Detected simple printf()-like format
* Adding c-format to "Add Logical Volume on %s"
  Detected simple printf()-like format
* Adding c-format to "Resize Volume Group %s"
  Detected simple printf()-like format
* Adding c-format to "Add RAID %s"
  Detected simple printf()-like format
* Adding c-format to "Edit devices of RAID %s"
  Detected simple printf()-like format
* Adding c-format to "It is not possible to create a new partition table..."
  Detected simple printf()-like format
* Adding c-format to "Really create a new partition table on %s"
  Detected simple printf()-like format
* Adding c-format to "This will delete all data from the %s file system ..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The device %{name} is a flash-only bcache. Deletin..."
  Detected simple printf()-like format
* Adding c-format to "Really delete %s and all the affected devices?"
  Detected simple printf()-like format
* Adding c-format to "Really delete %s?"
  Detected named parameters %{value}
* Adding perl-brace-format to "The thin pool %{name} is used by at least one thin..."
  Detected named parameters %{value}
* Adding perl-brace-format to "Really delete the thin pool "%{name}" and all rela..."
  Detected simple printf()-like format
* Adding c-format to "The volume group "%s" contains at least one logica..."
  Detected simple printf()-like format
* Adding c-format to "Really delete volume group "%s" and all related lo..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The device %{name} is directly formatted and\nand ..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The device %{name} does not contain a partition ta..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The device %{name} is a flash-only bcache and its ..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The bcache %{name} is already created on disk. Suc..."
  Detected simple printf()-like format
* Adding c-format to "Edit RAID %s"
  Detected named parameters %{value}
* Adding perl-brace-format to "Edit Logical Volume %{lv_name} on %{vg}"
  Detected simple printf()-like format
* Adding c-format to "Edit Partition %s"
  Detected simple printf()-like format
* Adding c-format to "Edit Device %s"
  Detected named parameters %{value}
* Adding perl-brace-format to "The device %{name} is in use (%{users}).\nIt canno..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The device %{name} belongs to a Btrfs.\nIt cannot ..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The device %{name} belongs to a multi-device files..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The device %{name} contains partitions.\nIt cannot..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The volume %{name} is a thin pool.\nIt cannot be e..."
  Detected named parameters %{value}
* Adding perl-brace-format to "Edit Btrfs %{basename}"
  Detected named parameters %{value}
* Adding perl-brace-format to "Edit devices of Btrfs %{name}"
  Detected named parameters %{value}
* Adding perl-brace-format to "The Btrfs %{name} is already created on disk and i..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The RAID %{name} is already created on disk and it..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The RAID %{name} is in use. To modify the used dev..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The RAID %{name} is partitioned. To modify the use..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The partition %{name} is already created on disk\n..."
  Detected named parameters %{value}
* Adding perl-brace-format to "No space to move partition %{name}"
  Detected named parameters %{value}
* Adding perl-brace-format to "The device %{name} is being used by:\n%{users}\n\n..."
  Detected named parameters %{value}
* Adding perl-brace-format to "<p><b>%{label}</b> is the device that will be used..."
  Detected named parameters %{value}
* Adding perl-brace-format to "<p><b>%{label}</b> is the device that will be used..."
  Detected named parameters %{value}
* Adding perl-brace-format to "<p><b>%{label}</b> is the operating mode for bcach..."
  Detected named parameters %{value}
* Adding perl-brace-format to "Resize %{name}"
  Detected named parameters %{value}
* Adding perl-brace-format to "Current size: %{size}"
  Detected named parameters %{value}
* Adding perl-brace-format to "Part of %{btrfs}"
  Detected named parameters %{value}
* Adding perl-brace-format to "Currently used: %{size}"
  Detected sformat()-like positional parameters
* Adding ycp-format to "You are extending a mounted filesystem by %1 Gigab..."
  Detected named parameters %{value}
* Adding perl-brace-format to "Maximum Size (%{size})"
  Detected named parameters %{value}
* Adding perl-brace-format to "Minimum Size (%{size})"
  Detected named parameters %{value}
* Adding perl-brace-format to "The size entered is invalid. Enter a size between ..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The LVM thin pool %{name} is overcomitted (needs %..."
  Detected simple printf()-like format
* Adding c-format to "Subvolume name %s already exists."
  Detected simple printf()-like format
* Adding c-format to "Cannot create subvolume %s."
  Detected simple printf()-like format
* Adding c-format to "Delete subvolume %s first."
  Detected named parameters %{value}
* Adding perl-brace-format to "Only subvolume names starting with "%{prefix}" cur..."
  Detected simple printf()-like format
* Adding c-format to "Preserve existing encryption (%s)"
  Detected simple printf()-like format
* Adding c-format to "Maximum Size (%s)"
  Detected named parameters %{value}
* Adding perl-brace-format to "Move partition %{name} towards the beginning?"
  Detected named parameters %{value}
* Adding perl-brace-format to "Move partition %{name} towards the end?"
  Detected named parameters %{value}
* Adding perl-brace-format to "Move partition %{name}?"
  Detected sformat()-like positional parameters
* Adding ycp-format to "Maximum Size (%1)"
  Detected sformat()-like positional parameters
* Adding ycp-format to "The size entered is invalid. Enter a size between ..."
  Detected named parameters %{value}
* Adding perl-brace-format to "Clone partition layout of %{name}"
  Detected simple printf()-like format
* Adding c-format to "Create New Partition Table on %s"
  Detected named parameters %{value}
* Adding perl-brace-format to "Your %{name} device is very small for snapshots.\n..."
  Detected sformat()-like positional parameters
* Adding ycp-format to "The file system is currently mounted on %1."
  Detected simple printf()-like format
* Adding c-format to "Device: %s"
  Detected simple printf()-like format
* Adding c-format to "Size: %s"
  Detected simple printf()-like format
* Adding c-format to "Device Path %i: %s"
  Detected simple printf()-like format
* Adding c-format to "Device Path: %s"
  Detected simple printf()-like format
* Adding c-format to "Device ID %i: %s"
  Detected simple printf()-like format
* Adding c-format to "Device ID: %s"
  Detected simple printf()-like format
* Adding c-format to "Encrypted: Yes (%s)"
  Detected simple printf()-like format
* Adding c-format to "Vendor: %s"
  Detected simple printf()-like format
* Adding c-format to "Model: %s"
  Detected simple printf()-like format
* Adding c-format to "Bus: %s"
  Detected simple printf()-like format
* Adding c-format to "Number of Sectors: %i"
  Detected simple printf()-like format
* Adding c-format to "Sector Size: %s"
  Detected simple printf()-like format
* Adding c-format to "Partition Table: %s"
  Detected simple printf()-like format
* Adding c-format to "PV of %s"
  Detected simple printf()-like format
* Adding c-format to "Part of %s"
  Detected named parameters %{value}
* Adding perl-brace-format to "<p><b>%{label}:</b> RAID level for the Btrfs data...."
  Detected named parameters %{value}
* Adding perl-brace-format to "According to the selected devices, only the follow..."
  Detected named parameters %{value}
* Adding perl-brace-format to "<p><b>%{label}</b> Unused devices that can be used..."
  Detected named parameters %{value}
* Adding perl-brace-format to "<p><b>%{label}</b> Devices selected to be part of ..."
  Detected named parameters %{value}
* Adding perl-brace-format to "<p><b>%{label}:</b> RAID level for the Btrfs metad..."
  Detected simple printf()-like format
* Adding c-format to "Mount point %s is shadowed."
  Detected simple printf()-like format
* Adding c-format to "Really delete subvolume %s?"
  Detected simple printf()-like format
* Adding c-format to "Backing Device: %s"
  Detected simple printf()-like format
* Adding c-format to "Caching UUID: %s"
  Detected simple printf()-like format
* Adding c-format to "Caching Device: %s"
  Detected simple printf()-like format
* Adding c-format to "Cache Mode: %s"
  Detected simple printf()-like format
* Adding c-format to "File System: %s"
  Detected simple printf()-like format
* Adding c-format to "Mount Point: %s"
  Detected simple printf()-like format
* Adding c-format to "Mount By: %s"
  Detected simple printf()-like format
* Adding c-format to "Label: %s"
  Detected simple printf()-like format
* Adding c-format to "UUID: %s"
  Detected simple printf()-like format
* Adding c-format to "RAID Level: %s"
  Detected simple printf()-like format
* Adding c-format to "Metadata RAID Level: %s"
  Detected simple printf()-like format
* Adding c-format to "Journal Device: %s"
  Detected simple printf()-like format
* Adding c-format to "PE Size: %s"
  Detected simple printf()-like format
* Adding c-format to "Active: %s"
  Detected simple printf()-like format
* Adding c-format to "RAID Type: %s"
  Detected simple printf()-like format
* Adding c-format to "Chunk Size: %s"
  Detected simple printf()-like format
* Adding c-format to "Parity Algorithm: %s"
  Detected simple printf()-like format
* Adding c-format to "Partition ID: %s"
  Detected simple printf()-like format
* Adding c-format to "Resulting size: %s"
  Detected simple printf()-like format
* Adding c-format to "Total size: %s"
  Detected named parameters %{value}
* Adding perl-brace-format to "<p>%{header}</p><ul>%{content}</ul>"
  Detected named parameters %{value}
* Adding perl-brace-format to "<p><b>%{label}</b>: allows to encrypt the device u..."
  Detected named parameters %{value}
* Adding perl-brace-format to "<p><b>%{label}</b>: allows to encrypt the device u..."
  Detected named parameters %{value}
* Adding perl-brace-format to "<p><b>%{label}</b>: this encryption method uses ra..."
  Detected named parameters %{value}
* Adding perl-brace-format to "<p><b>%{label}</b>: this encryption method uses a ..."
  Detected named parameters %{value}
* Adding perl-brace-format to "<p><b>%{label}</b>: this encryption method uses a ..."
  Detected simple printf()-like format
* Adding c-format to "The Btrfs subvolume mounted at %s is shadowed."
  Detected simple printf()-like format
* Adding c-format to "Stripes: %s"
  Detected simple printf()-like format
* Adding c-format to "%s (%s)"
  Detected simple printf()-like format
* Adding c-format to "The volume group size cannot be less than %s."
  Detected named parameters %{value}
* Adding perl-brace-format to "Removing physical volumes %{pvs} from the volume g..."
  Detected named parameters %{value}
* Adding perl-brace-format to "Removing the physical volume %{pvs} from the volum..."
  Detected named parameters %{value}
* Adding perl-brace-format to "For %{raid_level}, select at least %{min} devices."
  Detected simple printf()-like format
* Adding c-format to "Bcache: %s"
  Detected named parameters %{value}
* Adding perl-brace-format to "Btrfs %{basename}"
  Detected simple printf()-like format
* Adding c-format to "Hard Disk: %s"
  Detected simple printf()-like format
* Adding c-format to "Logical Volume: %s"
  Detected simple printf()-like format
* Adding c-format to "Volume Group: %s"
  Detected simple printf()-like format
* Adding c-format to "RAID: %s"
  Detected simple printf()-like format
* Adding c-format to "NFS configuration is not available. Check %s packa..."
  Detected simple printf()-like format
* Adding c-format to "Partition: %s"
  Detected simple printf()-like format
* Adding c-format to "Available Storage on %s"
  Detected simple printf()-like format
* Adding c-format to "Test mount of NFS share '%s' failed.\nSave it anyw..."
  Detected simple printf()-like format
* Adding c-format to "%d subvolume actions (<a href="%s">see details</a>..."
  Detected simple printf()-like format
* Adding c-format to "%d subvolume actions (<a href="%s">hide details</a..."
  Detected simple printf()-like format
* Adding c-format to "A problem ocurred while creating the partitioning ..."
  Detected named parameters %{value}
* Adding perl-brace-format to "Invalid value '%{value}' for attribute '%{attr}' (..."
  Detected named parameters %{value}
* Adding perl-brace-format to "replaced by '%{value}'"
  Detected named parameters %{value}
* Adding perl-brace-format to "Missing element '%{attr}'"
  Detected named parameters %{value}
* Adding perl-brace-format to "%{bcache_name}: only one %{role} can be specified ..."
  Detected named parameters %{value}
* Adding perl-brace-format to "Could not find a suitable device for Btrfs filesys..."
  Detected named parameters %{value}
* Adding perl-brace-format to "Could not find a suitable physical volume for volu..."
  Detected named parameters %{value}
* Adding perl-brace-format to "Could not find a suitable member for RAID '%{name}..."
  Detected named parameters %{value}
* Adding perl-brace-format to "Could not find a backing device for Bcache '%{name..."
  Detected simple printf()-like format
* Adding c-format to "Disk '%s' was not found"
  Detected named parameters %{value}
* Adding perl-brace-format to "The device '%{device}' cannot contain a partition ..."
  Detected simple printf()-like format
* Adding c-format to "No suitable device was found, none of the remainin..."
  Detected named parameters %{value}
* Adding perl-brace-format to "Some additional space (%{diff}) was required for n..."
  Detected named parameters %{value}
* Adding perl-brace-format to "%{identifier} to %{size} (-%{diff})"
  Detected simple printf()-like format
* Adding c-format to "Thin pool '%s' was not found"
  Detected named parameters %{value}
* Adding perl-brace-format to "A drive of type '%{type}' is not supported in this..."
  Detected simple printf()-like format
* Adding c-format to "Cache set (%s)"
  Detected simple printf()-like format
* Adding c-format to "Not enough space before the first partition to ins..."
  Detected simple printf()-like format
* Adding c-format to "A partition of type %s is needed to install the bo..."
  Detected simple printf()-like format
* Adding c-format to "The following PReP partition is too big: %s. "
  Detected simple printf()-like format
* Adding c-format to "Some firmwares can fail to load PReP partitions bi..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The encrypted volume could not be activated (attem..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The storage subsystem is locked by the application..."
  Detected simple printf()-like format
* Adding c-format to "This is not enabled at this moment (event: %s)"
  Detected named parameters %{value}
* Adding perl-brace-format to "The volume group %{name} is incomplete because som..."
  Detected named parameters %{value}
* Adding perl-brace-format to "The volume group %{name} is incomplete because som..."
  Detected named parameters %{value}
* Adding perl-brace-format to "Device Name: %{name}"
  Detected named parameters %{value}
* Adding perl-brace-format to "LUKS UUID: %{uuid}"
  Detected simple printf()-like format
* Adding c-format to "Select one or more (max %d) hard disks"
  Detected simple printf()-like format
* Adding c-format to "At most %d disks can be selected"
  Detected named parameters %{value}
* Adding perl-brace-format to "%{widget_label}\n(%{paths})"
  Detected simple printf()-like format
* Adding c-format to "The password must have at least %d characters."
  Detected named parameters %{value}
* Adding perl-brace-format to "(%{basename}\u2026)"
  Detected named parameters %{value}
* Adding perl-brace-format to "Btrfs over %{num_devices} devices %{name}"
  Detected named parameters %{value}
* Adding perl-brace-format to "Unused LVM PV on %{device}"
  Detected simple printf()-like format
* Adding c-format to "Adding the following packages failed: %s"
  Detected simple printf()-like format
* Adding c-format to "do not propose a separate %s"
  Detected simple printf()-like format
* Adding c-format to "disable snapshots and RAM-based size adjustments f..."
  Detected simple printf()-like format
* Adding c-format to "do not enable snapshots for %s"
  Detected simple printf()-like format
* Adding c-format to "do not adjust size of %s based on RAM size"
storage.pot: 177 format flags added
Writing storage.pot

Package Dependencies

Like the related y2makepot, this script uses rubygem-gettext. It relies on the weak dependencies in the .spec file (a suggests) and in the y2makepot invocation.

The rationale is that most developers who need this yast-devtools package never generate .pot files, so it should not be a hard requirement.

@lslezak
Copy link
Member

lslezak commented May 4, 2020

Post-processing of the POT files looks like a simpler solution than extending the GNU gettext, IMHO a nice solution! 👍

Note: maybe we should support a special comment which would disable this heuristic for specific texts. I do not have any good example, but in theory it might happen that the %s or %1 might be used outside of format(_(...)) or Builtins.format(_(...)) calls.

@shundhammer
Copy link
Contributor Author

shundhammer commented May 4, 2020

Note: maybe we should support a special comment which would disable this heuristic for specific texts. I do not have any good example, but in theory it might happen that the %s or %1 might be used outside of format(_(...)) or Builtins.format(_(...)) calls.

That already exists:

# xgettext: c-format
foo(_("...%1..."))

Considering already existing formats is one of the "rough edges" that I mentioned above: If a message already has a format, we should not add another one, let alone a different one.

Edit: The script will now not touch a message that already has a format.

@shundhammer
Copy link
Contributor Author

shundhammer commented May 5, 2020

Notice that the Travis build fails because it uses the previous yast-devtools which don't yet include the fix for y2makepot to exclude this new script from extracting textdomains. We will have to override that for now and check in a red Travis build.

Edit: After renaming the script (now without the .rb extension), this is no longer a problem since the yast-devtools only scan *.rb files.

@shundhammer shundhammer changed the title WIP: Add Format Markers Add Format Markers May 5, 2020
@shundhammer shundhammer requested a review from lslezak May 6, 2020 07:21
Copy link
Member

@lslezak lslezak left a comment

Choose a reason for hiding this comment

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

Just some minor comments

build-tools/scripts/po_add_format_hints.rb Outdated Show resolved Hide resolved
build-tools/scripts/po_add_format_hints.rb Outdated Show resolved Hide resolved
build-tools/scripts/po_add_format_hints.rb Outdated Show resolved Hide resolved
build-tools/scripts/po_add_format_hints.rb Outdated Show resolved Hide resolved
build-tools/scripts/po_add_format_hints.rb Outdated Show resolved Hide resolved
build-tools/scripts/po_add_format_hints.rb Outdated Show resolved Hide resolved
build-tools/scripts/po_add_format_hints.rb Outdated Show resolved Hide resolved
@mvidner
Copy link
Member

mvidner commented May 6, 2020

Thanks for the excellent PR description!

IMHO the program name should not include the .rb extension which is an implementation detail. Unlikely that we will ever reimplement it in a different language, but to be consistent with the rest of scripts/ which already mixes Bash, Ruby and Perl without the extensions.

@mvidner
Copy link
Member

mvidner commented May 6, 2020

RSpec unit tests in tests/ in this repo. Those tests are not integrated into the Autotools build logic of this project.

That's understandable, since we don't want to enlarge the build deps on a critical build path, but unfortunate. I think adding the test and skipping it if Ruby+RSpec is not available is better. I will make a patch.

@mvidner
Copy link
Member

mvidner commented May 6, 2020

Here: 4ef3308

Copy link
Member

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

Thanks!

Please apply my commits for the tests. Removing the .rb extension would be nice to have.

@shundhammer shundhammer requested a review from mvidner May 6, 2020 14:40
@shundhammer
Copy link
Contributor Author

After all the changes (in particular renaming the script), I just put it through another real-world test with rake pot in yast-storage-ng, and it still works fine.

@shundhammer shundhammer merged commit 4dcb03a into yast:master May 6, 2020
@yast-bot
Copy link
Contributor

yast-bot commented May 6, 2020

✔️ Public Jenkins job #18 successfully finished
✔️ Created OBS submit request #800787

@yast-bot
Copy link
Contributor

✔️ Internal Jenkins job #7 successfully finished
✔️ Created IBS submit request #224082

@yast-bot
Copy link
Contributor

✔️ Internal Jenkins job #8 successfully finished
✔️ Created IBS submit request #226247

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.

4 participants