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 Toc Translation #96

Closed
wants to merge 1 commit into from
Closed

Conversation

wanghaosjtu
Copy link
Contributor

Translate ToCs in .ncx file. Test with 6.22
but have run time error in 5.36
File "calibre_plugins.ebook_translator.lib.async_handler", line 58, in process_tasks
not related to ToC translation.

bookfere added a commit that referenced this pull request Jul 20, 2023
@bookfere
Copy link
Owner

Thank you for your PR.

I have reviewed your code carefully, and I am wondering if it is necessary to handle TOC and PAGES separately. What if we put items obtained from them, wrapped with different types but derived from the same Element type, into the same container? I think this approach will be more maintainable and make it easier to write test cases for the code.

By the way, I would greatly appreciate it if your code follows the PEP8 style guide and includes test cases for the core functionality you added.

@wanghaosjtu
Copy link
Contributor Author

wanghaosjtu commented Jul 24, 2023

if it is necessary to handle TOC and PAGES separately

如果用``add without original'' ,就很难找到在读哪篇了,比如法语或者俄语的时候
pages在oeb.manifest.items里,而toc在oeb.toc里,你愿意re-arch Extraction么?

follows the PEP8 style guide

我pylint就可以吧?

includes test cases for the core functionality you added.

我没用过pytest。可以试试从tests/test_element.py开始?

Sorry for late response due to access issue

@bookfere
Copy link
Owner

In my opinion, the inheritance approach you supplied reimplements all of the primary methods, which means we cannot gain the benefits of reusing code from inheritance. Additionally, we have to add almost duplicated test cases for it, resulting in additional effort for maintenance.

So, I suppose we can use one single logic to handle both TOC items and PAGE elements. Here is a demonstration code without verification:

class Element:
    """Shared common methods and abstract methods will be added here."""

class TocElement(Element):
    """Specific codes for TOC items. Get original and add translation."""

class PageElement(Element):
    """Specific codes for PAGE elements. Get original and add translation."""

class ElementHandler:
    """We can handle both TOC items and PAGE elements with the same logic."""

If the demonstration above can be implemented, we just need to add test cases for the TocElement we newly added, instead of adding test cases that are almost the same as those for ElementHandler for the TocElementHandler.

We can see that all of the TOC data was stored in the collection oeb.toc.nodes, so we need not modify Extraction; instead, we just need to wrap each item in nodes into TocElement and merge them with PAGE elements to pass them to ElementHandler.

These are my initial thoughts after reviewing your PR. What do you think?


Sure, you can use whatever tools you prefer to reach the purpose we discussed.

This project uses the Python built-in unit testing framework unittest as the testing tool. For this project, you can run the command as follows to execute all existing test cases:

calibre-customize -b .; calibre-debug test.py

By the way, which is your preferred language, English or Chinese? I can use that to communicate with you.

@bookfere
Copy link
Owner

在我看来,你提供的继承方式重新实现了所有主要方法,这意味着我们无法从继承上获得代码重用的好处。额外的,我们还需要为其添加几乎是重复的测试用例,导致在维护上的额外投入。

因此,我假设我们可以使用同一逻辑来处理目录项目和页面元素。这是一个未经验证的示例代码:

class Element:
    """共享的通用方法和抽象方法添加到这里。"""

class TocElement(Element):
    """特定于目录项目的代码。获取原文和添加译文。"""

class PageElement(Element):
    """特定于页面元素的代码。获取原文和添加译文。"""

class ElementHandler:
    """我们可以用同一逻辑处理目录项目和页面元素。"""

如果上面这个示例可以实现的话,我们不必为 TocElementHandler 添加几乎与 ElementHandler 相同的测试用例,而只需要为新添加的 TocElement 添加测试用例就可以了。

我们可以看到所有的目录数据都被存储到了集合 oeb.toc.nodes 中,因此我们不需要修改 Extraction,而是将 nodes 中的项目包在 TocElement 中,然后将其和页面元素合并传送给 ElementHandler

这些是我在检查你的 PR 后的初步想法。你怎么认为?


当然,你可以使用任何你喜欢的工具实现我们的讨论要达成的目的。

此项目使用 Python 内置单元测试框架 unittest 作为测试工具。对于这个项目,你可以运行以下命令来执行现有的测试用例:

calibre-customize -b .; calibre-debug test.py

另外,你的首选语言是哪个,英语还是中文?我可以用其一和你交流。

@bookfere bookfere closed this in 3a2d943 Sep 17, 2023
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.

2 participants