Addressing problems raised in sphinx_issues #10104 #863

hoangduytranuk · 2022-04-23T07:21:02Z

branch

master

purpose

Addressed several issues with PYTHON_FORMAT pattern
Addressed issues with comment blocks

details

The PYTHON_FORMAT pattern wrongly found incidences of percentage sign, such as 50% or %28c in links, and flagging the message as python-format, which is exposed under the use of editors such as PoEdit as a wrong use of python-format feature, which many translators use. This editor is RECOMMENDED by Blender.org for use in the internationalisation projects.
Problem is solved by added look behind to see if it is not a space character and look ahead to see if the character ended the pattern is one of the allowed symbols - note: many will probably needed to manually enter to allow expansion of the use of '%s %d' etc. - I have only added characters that were found in one of the blender's po file vi.po file. One can download this file to test and simply use the following Python code:

from sphinx_intl import catalog as c
home = os.environ['HOME']
file_path = os.path.join(home, 'vi.po')
out_file = os.path.join(home, 'test_vi.po')
data = c.load_po(file_path)
c.dump_po(out_file, data, line_width=4096)

The original file doesn't contain any 'python-format', after the test, there should be 582 entries flagged as python-format, if load into the PoEditor, there should not be any PINK flagging indicating misuse of python-format.

Comment blocks are particularly good use for developers and translators as they are the quick reference to where the line originated from (which source files) and what comments developers had placed in to notify others. For this reason, several issues are address here:
- Remove duplications in the entries
- Sort entries alphabetically for easer spotting and locating the origins
- Remove the wrapping by line length to the comment block as this SHOULD NOT be done. The observation that xgettext also performing wrapping is a fallacy as test shown using 'msgmerge --no-wrap' doesn't always wrap every lines, only a few incidences are found, indicating there is an error in the original xgettext code. This wrapping is particularly annoying for developers observing changes in the po files in a repository. Only physical changes to either msgid or msgstr should be observed, not the comment block, especially when there are no changes and the flagging of changes is created by the effects of wrapping functionality of the software.

link to the issues:

Message.locations duplicate unnecessary #10104

… the issue 10104 by simlify in down to checking for list instance of the input and making sure that they are.

…pler

akx

Beyond the inline comments:

can you clean up the commits and commit messages?
could you add tests that prove these changes do what they should?

akx · 2022-05-10T06:42:18Z

babel/messages/catalog.py

-        self.locations = list(distinct(locations))
+        self.locations = list(set(locations))


Why is this changed? Using set will lose the original order of locations.

Thank you so much for your comments. Please read my original post here (Message.locations duplicate unnecessary) for the original observations and tests performed. I just personally found that locations will need to be sorted in alphabetical order for user to easily observing and finding the file, especially when the location list is long. so to make it unique using 'set' would not affecting the performance and the correctness.

This is one example of location list in the Blender

#: ../../manual/about/contribute/guides/markup_guide.rst:143 #: ../../manual/addons/3d_view/3d_navigation.rst #: ../../manual/addons/3d_view/math_vis_console.rst #: ../../manual/addons/3d_view/measureit.rst #: ../../manual/addons/3d_view/precision_drawing_tools/index.rst #: ../../manual/addons/3d_view/stored_views.rst #: ../../manual/addons/3d_view/vr_scene_inspection.rst #: ../../manual/addons/add_curve/assign_shape_keys.rst #: ../../manual/addons/add_curve/btracer.rst #: ../../manual/addons/add_curve/curve_tools.rst #: ../../manual/addons/add_curve/extra_objects.rst #: ../../manual/addons/add_curve/ivy_gen.rst #: ../../manual/addons/add_curve/sapling.rst #: ../../manual/addons/add_curve/simplify_curves.rst #: ../../manual/addons/add_mesh/ant_landscape.rst #: ../../manual/addons/add_mesh/archimesh.rst #: ../../manual/addons/add_mesh/boltfactory.rst #: ../../manual/addons/add_mesh/discombobulator.rst #: ../../manual/addons/add_mesh/geodesic_domes.rst #: ../../manual/addons/add_mesh/mesh_extra_objects.rst #: ../../manual/addons/animation/animall.rst #: ../../manual/addons/animation/bone_selection_sets.rst #: ../../manual/addons/animation/corrective_shape_keys.rst #: ../../manual/addons/animation/turnaround_camera.rst #: ../../manual/addons/camera/camera_rigs.rst #: ../../manual/addons/development/dependency_graph.rst #: ../../manual/addons/development/edit_operator.rst #: ../../manual/addons/development/icon_viewer.rst #: ../../manual/addons/development/is_key_free.rst #: ../../manual/addons/interface/amaranth.rst #: ../../manual/addons/interface/brush_menus.rst #: ../../manual/addons/interface/collection_manager.rst #: ../../manual/addons/interface/context_menu.rst #: ../../manual/addons/interface/copy_attributes.rst #: ../../manual/addons/interface/modifier_tools.rst #: ../../manual/addons/interface/viewport_pies.rst #: ../../manual/addons/lighting/dynamic_sky.rst #: ../../manual/addons/lighting/sun_position.rst #: ../../manual/addons/lighting/sun_position.rst:95 #: ../../manual/addons/lighting/trilighting.rst #: ../../manual/addons/materials/material_library.rst #: ../../manual/addons/materials/material_utils.rst #: ../../manual/addons/mesh/3d_print_toolbox.rst #: ../../manual/addons/mesh/auto_mirror.rst #: ../../manual/addons/mesh/bsurfaces.rst #: ../../manual/addons/mesh/edit_mesh_tools.rst #: ../../manual/addons/mesh/f2.rst #: ../../manual/addons/mesh/inset_straight_skeleton.rst #: ../../manual/addons/mesh/looptools.rst #: ../../manual/addons/mesh/relax.rst #: ../../manual/addons/mesh/snap_utilities_line.rst #: ../../manual/addons/mesh/tinycad.rst #: ../../manual/addons/mesh/tissue.rst #: ../../manual/addons/node/node_arrange.rst #: ../../manual/addons/node/node_presets.rst #: ../../manual/addons/node/node_wrangler.rst #: ../../manual/addons/object/align_tools.rst #: ../../manual/addons/object/bool_tools.rst #: ../../manual/addons/object/carver.rst #: ../../manual/addons/object/cell_fracture.rst #: ../../manual/addons/object/color_rules.rst #: ../../manual/addons/object/edit_linked_library.rst #: ../../manual/addons/object/greasepencil_tools.rst #: ../../manual/addons/object/real_snow.rst #: ../../manual/addons/object/scatter_objects.rst #: ../../manual/addons/object/skinify.rst #: ../../manual/addons/paint/paint_palettes.rst #: ../../manual/addons/render/copy_settings.rst #: ../../manual/addons/render/povray.rst #: ../../manual/addons/rigging/rigify/index.rst #: ../../manual/addons/sequencer/power_sequencer.rst #: ../../manual/addons/system/blend_info.rst #: ../../manual/addons/system/blender_id.rst #: ../../manual/addons/system/demo_mode.rst #: ../../manual/addons/system/property_chart.rst #: ../../manual/addons/system/ui_translations.rst #: ../../manual/addons/uv/magic_uv.rst #: ../../manual/addons/video_tools/refine_tracking.rst #: ../../manual/animation/constraints/relationship/child_of.rst:42 #: ../../manual/editors/3dview/3d_cursor.rst:60 #: ../../manual/editors/3dview/sidebar.rst:98 #: ../../manual/grease_pencil/materials/properties.rst:164 #: ../../manual/grease_pencil/materials/properties.rst:189 #: ../../manual/modeling/geometry_nodes/input/object_info.rst:46 #: ../../manual/movie_clip/tracking/clip/sidebar/stabilization/panel.rst:55 #: ../../manual/physics/forces/force_fields/introduction.rst:116 #: ../../manual/render/shader_nodes/input/object_info.rst:33 #: ../../manual/render/shader_nodes/input/particle_info.rst:48 #: ../../manual/render/shader_nodes/input/point_info.rst:33 #: ../../manual/render/shader_nodes/vector/mapping.rst:23 #: ../../manual/scene_layout/object/editing/apply.rst:45 #: ../../manual/scene_layout/object/editing/transform/randomize.rst:32 #: ../../manual/scene_layout/object/properties/transforms.rst:33 #: ../../manual/scene_layout/object/types.rst:99 msgid "Location" msgstr "Vị Trí (Location)"

The original document can be found here

Blender UI translation SVN Repository

You can just run

svn co https://svn.blender.org/svnroot/bf-translations/trunk/po

and take a look at the file. As a translator, I have occasionally observing changes from the 'blender.pot' file and I always wished this location data block is made distinct and sorted alphabetically.

akx · 2022-05-10T06:43:12Z

babel/messages/pofile.py

@@ -276,6 +276,7 @@ def _process_comment(self, line):
                    self.locations.append((location[:pos], lineno))
                else:
                    self.locations.append((location, None))
+            self.locations = list(set(self.locations))


Same here, this should probably also use distinct. Also, can you explain what's happening here? Why is self.locations modified every time comments are processed?

Thank you, you're right, don't need to perform this here. When the message is created then do it there instead, once and for all. Thank you again.

akx · 2022-05-10T06:43:21Z

babel/messages/pofile.py

+<<<<<<< HEAD
+    def _write_comment(comment, prefix='':
+=======


Merge conflict marker and syntax error.

I don't know why there is an error here, the code at my local appeared to be OK. I will make another push to ensure the correct code is placed.

akx · 2022-05-10T06:43:28Z

babel/messages/pofile.py

-        else:
-            _width = 76
-        for line in wraptext(comment, _width):
+>>>>>>> 67aa652a1c10bfdc92587cdae87fdab65d633b23


Merge conflict marker.

These lines are removed. There is no need for wrapping comments any more. Please refer back to the original post commented above

akx · 2022-05-10T06:44:11Z

babel/messages/pofile.py

+        has_comment = (bool(comment) and len(comment) > 0)
+        if not has_comment:


I believe this simplifies to

Suggested change

has_comment = (bool(comment) and len(comment) > 0)

if not has_comment:

if not comment:

Thank you for this comment. I'm changing the code and pushing another commit.

akx · 2022-05-10T06:45:10Z

babel/messages/pofile.py

+        is_list_type = isinstance(comment, list)
+        comment_list = (list(comment) if not is_list_type else comment)
+        comment_list.sort()


Can you explain what's happening here? Is it possible this reorders multiline comments into the wrong order?

As I run the code in tests, there are times the comment came in as an instance of list, sometimes it is not. So to make sure they are all instance of list type, I test it first and ensure that it is a list type whatever the input is, and sort the list according ly. Please run this simple testing code lines:

import os from sphinx_intl import catalog as c home = os.environ['HOME'] home_base = os.path.join(home, "<where ever the po directory is>" input_file = os.path.join(home_base, "blender.pot") output_file = os.path.join(home_base, "test_output_blender.pot") data = c.load_po(input_file) c.dump_po(output_file. data, line_width=4096)

and load the output file into an editor (ie. vscode) and POEdit to test for python-format errors.

akx · 2022-05-10T06:45:33Z

babel/messages/pofile.py

-        for comment in message.auto_comments:
-            _write_comment(comment, prefix='.')
+        _write_comment(message.user_comments)
+        _write_comment(message.auto_comments)


Why do we no longer use the '.' prefix here?

Thank you for this observation. I will be making a new commit for the corrected code.

akx · 2022-05-10T06:46:18Z

babel/messages/catalog.py

@@ -35,7 +35,7 @@
            (?:\.(?:\*|[\d]+))?
            [hlL]?
        )
-        ([diouxXeEfFgGcrs%])
+        ((?<!\s)[diouxXeEfFgGcrs%])(?=([\s\'\)\.\,\:\"\!\]\>\?]|$))


The commit message(s) don't seem to explain why these changes are necessary.

Please read the original post at

Message.locations duplicate unnecessary #10104

This is an extract of the original message:

By the way, in the past I posted a bug report mentioning the PYTHON_FORMAT problem, in that this re.Pattern causing the problem in recognizing this part "%50 'one letter'" (diouxXeEfFgGcrs%) as an ACCEPTABLE pattern, thus causing the flag "python_format" in the Message class to set, and the Catalog.dump_po will insert a "#, python-format" in the comment section of the message, causing applications such as PoEdit to flag up as a WRONG format for "python-format". The trick is to insert a look behind clause in the PYTHON_FORMAT pattern, as an example here:

After all the above comments, and as you finished your preview, could you please give me a approval as what other changes I should make, before I make another push to the pull request branch. Thank you.

…RMAL behaviour user expected: 1. PYTHON_FORMAT will correctly, with some degrees of limitation (haven't found with sampled test file of Blender UI translation file, but that doesn't mean it will not be broken again) identify '%s' '%d' flags in the messages and flag it as 'python-format', which removed mistaken recognitions of percentage sign, ie. '%50' as a python-format, which POEditor will flag up as an ERROR in the message. 2. Locations and comments are sorted Alphabetically and removed duplicated redundant lines. 3. Comment lines are NO LONGER wrapped with provided line_width. There appeared to be an error in the xgettext code when running with --no-wrap flag. Message bodies (ie. msgid, msgstr) should be wrapped but NOT comments. The wrapping can cause unnecessary version changes appeared in the repository submissions.

…RMAL behaviour user expected: 1. PYTHON_FORMAT will correctly, with some degrees of limitation (haven't found with sampled test file of Blender UI translation file, but that doesn't mean it will not be broken again) identify '%s' '%d' flags in the messages and flag it as 'python-format', which removed mistaken recognitions of percentage sign, ie. '%50' as a python-format, which POEditor will flag up as an ERROR in the message. 2. Locations and comments are sorted Alphabetically and removed duplicated redundant lines. 3. Comment lines are NO LONGER wrapped with provided line_width. There appeared to be an error in the xgettext code when running with --no-wrap flag. Message bodies (ie. msgid, msgstr) should be wrapped but NOT comments. The wrapping can cause unnecessary version changes appeared in the repository submissions. Resolved merge conflicts with previous changes

hoangduytranuk and others added 4 commits April 22, 2022 20:12

Addressing problems raised in sphinx_issues #10104

3258580

Corrected the unnecesary codes that was introduced in the last fix to…

67aa652

… the issue 10104 by simlify in down to checking for list instance of the input and making sure that they are.

Removed redundant codes in the last fix, making it more clean and sim…

53c89f4

…pler

Removed redundant codes in the last fix, making it more clean and sim…

7bddb4b

…pler

akx requested changes May 10, 2022

View reviewed changes

hoangduytranuk added 2 commits May 15, 2022 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addressing problems raised in sphinx_issues #10104 #863

Addressing problems raised in sphinx_issues #10104 #863

hoangduytranuk commented Apr 23, 2022

akx left a comment

akx May 10, 2022

ghost May 13, 2022

akx May 10, 2022

ghost May 15, 2022

akx May 10, 2022

ghost May 13, 2022

akx May 10, 2022

ghost May 13, 2022 •

edited by ghost

akx May 10, 2022

ghost May 13, 2022

akx May 10, 2022

ghost May 13, 2022

akx May 10, 2022

ghost May 13, 2022

akx May 10, 2022

ghost May 13, 2022

ghost May 13, 2022

		self.locations = list(distinct(locations))
		self.locations = list(set(locations))

		has_comment = (bool(comment) and len(comment) > 0)
		if not has_comment:

	has_comment = (bool(comment) and len(comment) > 0)
	if not has_comment:
	if not comment:

Addressing problems raised in sphinx_issues #10104 #863

Are you sure you want to change the base?

Addressing problems raised in sphinx_issues #10104 #863

Conversation

hoangduytranuk commented Apr 23, 2022

branch

purpose

details

link to the issues:

akx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost May 13, 2022 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost May 13, 2022 •

edited by ghost