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

Addressing problems raised in sphinx_issues #10104 #863

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Addressing problems raised in sphinx_issues #10104 #863

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3258580
Addressing problems raised in sphinx_issues #10104
Apr 22, 2022
67aa652
Corrected the unnecesary codes that was introduced in the last fix to…
hoangduytranuk Apr 24, 2022
53c89f4
Removed redundant codes in the last fix, making it more clean and sim…
hoangduytranuk Apr 24, 2022
7bddb4b
Removed redundant codes in the last fix, making it more clean and sim…
hoangduytranuk Apr 24, 2022
314b648
This fix propose following changes which can be significant to the NO…
May 15, 2022
90dd90d
This fix propose following changes which can be significant to the NO…
May 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions babel/messages/catalog.py
View file
Open in desktop
Expand Up @@ -35,7 +35,7 @@
(?:\.(?:\*|[\d]+))?
[hlL]?
)
([diouxXeEfFgGcrs%])
((?<!\s)[diouxXeEfFgGcrs%])(?=([\s\'\)\.\,\:\"\!\]\>\?]|$))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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:

Copy link

Choose a reason for hiding this comment

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

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.

''', re.VERBOSE)


Expand Down Expand Up @@ -94,7 +94,7 @@ def __init__(self, id, string=u'', locations=(), flags=(), auto_comments=(),
if not string and self.pluralizable:
string = (u'', u'')
self.string = string
self.locations = list(distinct(locations))
self.locations = list(set(locations))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

self.flags = set(flags)
if id and self.python_format:
self.flags.add('python-format')
Expand Down
33 changes: 19 additions & 14 deletions babel/messages/pofile.py
View file
Open in desktop
Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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.

elif line[1:].startswith(','):
for flag in line[2:].lstrip().split(','):
self.flags.append(flag.strip())
Expand Down Expand Up @@ -521,14 +522,21 @@ def _write(text):
text = text.encode(catalog.charset, 'backslashreplace')
fileobj.write(text)

<<<<<<< HEAD
def _write_comment(comment, prefix='':
=======
Copy link
Member

Choose a reason for hiding this comment

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

Merge conflict marker and syntax error.

Copy link

Choose a reason for hiding this comment

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

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.

def _write_comment(comment, prefix=''):
# xgettext always wraps comments even if --no-wrap is passed;
# provide the same behaviour
if width and width > 0:
_width = width
else:
_width = 76
for line in wraptext(comment, _width):
>>>>>>> 67aa652a1c10bfdc92587cdae87fdab65d633b23
Copy link
Member

Choose a reason for hiding this comment

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

Merge conflict marker.

Copy link

@ghost ghost May 13, 2022 •
edited by ghost

Choose a reason for hiding this comment

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

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

# NEVER wrap comments, this observation: "xgettext always wraps comments even if --no-wrap is passed;" is FALSE. There seemed to be a bug in the xgettext code, because wrapping doesn't always occur
# Make sure comments are unique and sorted alphabetically so locations can be easily searched and identify
has_comment = (bool(comment) and len(comment) > 0)
if not has_comment:
Copy link
Member

Choose a reason for hiding this comment

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

I believe this simplifies to

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

Copy link

Choose a reason for hiding this comment

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

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

return

is_list_type = isinstance(comment, list)
comment_list = (list(comment) if not is_list_type else comment)
comment_list.sort()
Comment on lines +530 to +532
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

for line in comment_list:
_write('#%s %s\n' % (prefix, line.strip()))

def _write_message(message, prefix=''):
Expand Down Expand Up @@ -577,10 +585,8 @@ def _write_message(message, prefix=''):
comment_header = u'\n'.join(lines)
_write(comment_header + u'\n')

for comment in message.user_comments:
_write_comment(comment)
for comment in message.auto_comments:
_write_comment(comment, prefix='.')
_write_comment(message.user_comments)
_write_comment(message.auto_comments)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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


if not no_location:
locs = []
Expand All @@ -602,7 +608,7 @@ def _write_message(message, prefix=''):
location = u'%s' % filename.replace(os.sep, '/')
if location not in locs:
locs.append(location)
_write_comment(' '.join(locs), prefix=':')
_write_comment(locs, prefix=':')
if message.flags:
_write('#%s\n' % ', '.join([''] + sorted(message.flags)))

Expand All @@ -622,8 +628,7 @@ def _write_message(message, prefix=''):
catalog.obsolete.values(),
sort_by=sort_by
):
for comment in message.user_comments:
_write_comment(comment)
_write_comment(message.user_comments)
_write_message(message, prefix='#~ ')
_write('\n')

Expand Down