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 #10379

Closed
wants to merge 2 commits into from
Closed

Addressing problems raised in sphinx_issues #10104 #10379

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9fa160a
Addressing problems raised in sphinx_issues #10104
Apr 22, 2022
ac71858
Fixed the issue #10104 after the code reviews from Adam Turner and Ta…
hoangduytranuk Apr 24, 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
6 changes: 3 additions & 3 deletions sphinx/builders/gettext.py
View file
Open in desktop
@@ -1,5 +1,5 @@
"""The MessageCatalogBuilder class."""

import os
Copy link
Member

Choose a reason for hiding this comment

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

Keep a blank line here

Suggested change
import os
import os

from codecs import open
from collections import OrderedDict, defaultdict
from datetime import datetime, timedelta, tzinfo
Expand Down Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

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

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})

self.uuids = uuids


Expand All @@ -57,7 +57,7 @@ def add(self, msg: str, origin: Union[Element, "MsgOrigin"]) -> None:

def __iter__(self) -> Generator[Message, None, None]:
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]]
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Added the class variable 'builder' to Catalog class

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

A

Copy link
Member

Choose a reason for hiding this comment

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

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?

uuids = [uuid for source, line, uuid in self.metadata[message]]
yield Message(message, positions, uuids)

Expand Down