Addressing problems raised in sphinx_issues #10104 #10379

hoangduytranuk · 2022-04-23T06:43:26Z

Subject: Solving problems raised in issues #10104

Breaking changes: master
Feature

Purpose

Allowing message locations to be sorted uniquely in alphabetic order. This will remove duplicates locations and enable easier search and identify locations for developers

Detail

remove duplicates locations
enabled easier search and identify locations for developers

Relates

Message.locations duplicate unnecessary

AA-Turner

A few comments:

A

AA-Turner · 2022-04-23T11:03:06Z

sphinx/builders/gettext.py

@@ -32,7 +32,7 @@ class Message:
    """An entry of translatable message."""
    def __init__(self, text: str, locations: List[Tuple[str, int]], uuids: List[str]):
        self.text = text
-        self.locations = locations
+        self.locations = list(set(locations))


I'd use sorted here -- set doesn't guarantee any sort order at a language level.

Suggested change

self.locations = list(set(locations))

self.locations = sorted({*locations})

AA-Turner · 2022-04-23T11:07:49Z

sphinx/builders/gettext.py

        for message in self.messages:
-            positions = [(source, line) for source, line, uuid in self.metadata[message]]
+            positions = [(os.path.relpath(source, start=os.getcwd()), line) for source, line, uuid in self.metadata[message]]


This would call os.getcwd() for every item in the loop. Does ommitting start= still give a usable result? Otherwise you could use cache os.getcwd() in a local.

Understandable change.

But this should be moved to the outside of this class because this class does not have appropriate directory information. I think what directory you want is the base directory of the document (a.k.a srcdir). But it seems you're using os.getcwd(). But it does not equal to srcdir. It usually equals to the confdir that contains conf.py, but not promised. It sometimes returns a different directory if 3rd party extension changes it.

BTW, please use sphinx.util.osutil:relpath() instead of os.path.relpath(). It will not be crashed even if path and start are on different drives on Windows.

Fixed the issue #10104 after the code reviews from Adam Turner and Takeshi Komiya:

Added the class variable 'builder' to Catalog class, so any instances of Catalog can access the information within the builder, including things such as app, config, srcdir, outdir etc.. see MessageCatalogBuilder for details.

Initialise this Catalog.builder as the instance of MessageCatalogBuilder is created, ie. in the init()

As Message class is created, list of locations passing to it will be made singular, using set(), sorted alphabetically for easier searching and identifying origins where the message can be found.

source dir is used within the iter of the Catalog, using sphinx.util.osutil.relpath, for safer functionality, to work out the relative path for each location entry.

Added the class variable 'builder' to Catalog class

Modifying global state like this is a bad idea, please could you refactor?

A

Yes, adding a class variable is not a good idea. Catalog class is a data class that represents message catalog. It is not related to the builder at all. So the path manipulation using builder should be called outside of the catalog class. How about processing just before calling GettextRenderer?

AA-Turner · 2022-04-23T11:08:44Z

sphinx/builders/gettext.py

@@ -1,5 +1,5 @@
 """The MessageCatalogBuilder class."""
-
+import os


Keep a blank line here

Suggested change

import os

import os

hoangduytranuk · 2022-04-24T12:42:29Z

Thank you so much for these feedbacks. I will return to fix the code at a later time and post another commit to the pull request. Best Regards, Hoang Tran

…

On 23 Apr 2022, at 12:09, Adam Turner ***@***.***> wrote: @AA-Turner requested changes on this pull request. A few comments: A In sphinx/builders/gettext.py <#10379 (comment)>: > @@ -32,7 +32,7 @@ class Message: """An entry of translatable message.""" def __init__(self, text: str, locations: List[Tuple[str, int]], uuids: List[str]): self.text = text - self.locations = locations + self.locations = list(set(locations)) I'd use sorted here -- set doesn't guarantee any sort order at a language level. ⬇️ Suggested change - self.locations = list(set(locations)) + self.locations = sorted({*locations}) In sphinx/builders/gettext.py <#10379 (comment)>: > for message in self.messages: - positions = [(source, line) for source, line, uuid in self.metadata[message]] + positions = [(os.path.relpath(source, start=os.getcwd()), line) for source, line, uuid in self.metadata[message]] This would call os.getcwd() for every item in the loop. Does ommitting start= still give a usable result? Otherwise you could use cache os.getcwd() in a local. In sphinx/builders/gettext.py <#10379 (comment)>: > @@ -1,5 +1,5 @@ """The MessageCatalogBuilder class.""" - +import os Keep a blank line here ⬇️ Suggested change -import os + +import os — Reply to this email directly, view it on GitHub <#10379 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKIONBYYLLCUQ4CPX5HMNNDVGPK6RANCNFSM5UEIUEYQ>. You are receiving this because you authored the thread.

…ner and Takeshi Komiya: - Added the class variable 'builder' to Catalog class, so any instances of Catalog can access the information within the builder, including things such as app, config, srcdir, outdir etc.. see MessageCatalogBuilder for details. - Initialise this Catalog.builder as the instance of MessageCatalogBuilder is created, ie. in the init() - As Message class is created, list of locations passing to it will be made singular, using set(), sorted alphabetically for easier searching and identifying origins where the message can be found. - source dir is used within the __iter__ of the Catalog, using sphinx.util.osutil.relpath, for safer functionality, to work out the relative path for each location entry.

AA-Turner · 2022-04-24T17:23:44Z

sphinx/builders/gettext.py

@@ -32,13 +32,14 @@ class Message:
    """An entry of translatable message."""
    def __init__(self, text: str, locations: List[Tuple[str, int]], uuids: List[str]):
        self.text = text
-        self.locations = locations
+        self.locations = list(sorted(list(set(locations))))     #make list unique and sorted alphabetically


sorted returns a list and can take a set as input

Suggested change

self.locations = list(sorted(list(set(locations)))) #make list unique and sorted alphabetically

self.locations = sorted(set(locations)) # sort and de-duplicate the locations list

tk0miya · 2022-05-26T00:26:59Z

Finally, I reimplemented this as #10104 and merged it. Thank you for your contribution!

ghost · 2022-06-04T12:55:41Z

I would like to add a little comment as I only found out today after using

catalog.dump_po(out_path, data, line_width=4069, sort_output=self.is_sorted)
I found that user comments are repeated and I had to enter the following block of code to change the behaviour:

def _write_comment(comment, prefix=''):
        # 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:
            return

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

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

to remove duplicated user comments

ghost · 2022-06-05T13:59:28Z

Sorry the above solution is NOT perfect yet. I have just found another incident where the

message.user_comments 
message.auto_comments

are duplicated, by running

msgmerge vi.po inkscape.pot -o test_merge.po

these two files are in the following links:

https://gitlab.com/inkscape/inkscape/-/jobs/2544777172/artifacts/file/po/inkscape.pot
https://gitlab.com/hoangduytran1960/inkscape/-/blob/master/po/vi.po

The output file

test_merge.po

The using the Sphinx code to flat out the comment sections and msgid, msgstr using:

data = c.load_po(self.po_path)
c.dump_po(out_path, data, line_width=4069, sort_output=self.is_sorted)

The result file shown this line is repeated twice:

# TRANSLATORS: "Swatches" -> color samples

I have to use the PyCharmIDE to debug and found out, so the correction code is this:

sort_by = None
    if sort_output:
        sort_by = "message"
    elif sort_by_file:
        sort_by = "location"

    for message in _sort_messages(catalog, sort_by=sort_by):
        if not message.id:  # This is the header "message"
            if omit_header:
                continue
            comment_header = catalog.header_comment
            if width and width > 0:
                lines = []
                for line in comment_header.splitlines():
                    lines += wraptext(line, width=width,
                                      subsequent_indent='# ')
                comment_header = u'\n'.join(lines)
            _write(comment_header + u'\n')

        **comment_list = []
        comment_list.extend(message.user_comments)
        comment_list.extend(message.auto_comments)
        comment_list_unique = list(set(comment_list))
        comment_list_unique.sort()**

        _write_comment(comment_list_unique)

the section surrounded by '**". This is an extraction from the file

site-packages/babel/messages/pofile.py

routine:

def _write_message(message, prefix=''):

Ok, I'm out.

Addressing problems raised in sphinx_issues sphinx-doc#10104

9fa160a

AA-Turner requested changes Apr 23, 2022

View reviewed changes

tk0miya added type:proposal a feature suggestion internals:internationalisation labels Apr 24, 2022

tk0miya added this to the 5.0.0 milestone Apr 24, 2022

AA-Turner reviewed Apr 24, 2022

View reviewed changes

tk0miya modified the milestones: 5.0.0, 5.x May 7, 2022

tk0miya modified the milestones: 5.x, 5.1.0 May 15, 2022

tk0miya modified the milestones: 5.1.0, 5.0.0 May 22, 2022

tk0miya closed this May 26, 2022

github-actions bot locked as resolved and limited conversation to collaborators Jul 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addressing problems raised in sphinx_issues #10104 #10379

Addressing problems raised in sphinx_issues #10104 #10379

hoangduytranuk commented Apr 23, 2022

AA-Turner left a comment

AA-Turner Apr 23, 2022

AA-Turner Apr 23, 2022

tk0miya Apr 24, 2022

hoangduytranuk Apr 24, 2022

AA-Turner Apr 24, 2022

tk0miya May 2, 2022

AA-Turner Apr 23, 2022

hoangduytranuk commented Apr 24, 2022 via email

AA-Turner Apr 24, 2022 •

edited

tk0miya commented May 26, 2022

ghost commented Jun 4, 2022 •

edited by ghost

ghost commented Jun 5, 2022 •

edited by ghost

	self.locations = list(set(locations))
	self.locations = sorted({*locations})

		@@ -1,5 +1,5 @@
		"""The MessageCatalogBuilder class."""

		import os

	self.locations = list(sorted(list(set(locations)))) #make list unique and sorted alphabetically
	self.locations = sorted(set(locations)) # sort and de-duplicate the locations list

Addressing problems raised in sphinx_issues #10104 #10379

Addressing problems raised in sphinx_issues #10104 #10379

Conversation

hoangduytranuk commented Apr 23, 2022

Purpose

Detail

Relates

AA-Turner left a comment

Choose a reason for hiding this comment

AA-Turner Apr 23, 2022

Choose a reason for hiding this comment

AA-Turner Apr 23, 2022

Choose a reason for hiding this comment

tk0miya Apr 24, 2022

Choose a reason for hiding this comment

hoangduytranuk Apr 24, 2022

Choose a reason for hiding this comment

AA-Turner Apr 24, 2022

Choose a reason for hiding this comment

tk0miya May 2, 2022

Choose a reason for hiding this comment

AA-Turner Apr 23, 2022

Choose a reason for hiding this comment

hoangduytranuk commented Apr 24, 2022 via email

AA-Turner Apr 24, 2022 • edited

Choose a reason for hiding this comment

tk0miya commented May 26, 2022

ghost commented Jun 4, 2022 • edited by ghost

ghost commented Jun 5, 2022 • edited by ghost

AA-Turner Apr 24, 2022 •

edited

ghost commented Jun 4, 2022 •

edited by ghost

ghost commented Jun 5, 2022 •

edited by ghost