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

Use the default Jieba dict for Chinese search if not set #13005

Merged
merged 8 commits into from
Oct 19, 2024
Merged

Use the default Jieba dict for Chinese search if not set #13005

Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4b211b2
Fallback to default dict if not set
Snoopy1866 Oct 11, 2024
b41d2fc
directly get the default dict path
Snoopy1866 Oct 11, 2024
46753b9
format
Snoopy1866 Oct 11, 2024
6ca14b6
single quote
Snoopy1866 Oct 11, 2024
7f05775
not rely on __file__
Snoopy1866 Oct 15, 2024
f16ea7b
Merge branch 'master' into feat-search-zh-jieba-fallback
Snoopy1866 Oct 18, 2024
2b69df3
Revert "not rely on __file__"
Snoopy1866 Oct 18, 2024
1276ec7
dynamically get default dict name
Snoopy1866 Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion sphinx/search/zh.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from __future__ import annotations

import inspect
Copy link
Member

Choose a reason for hiding this comment

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

Can we have this import in the function it is being used just to save a bit of import time (unless this is already imported by some other dependency)?

Copy link
Member

Choose a reason for hiding this comment

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

Why use inspect here? It's a very heavy package, we can just use .__file__?

Copy link
Member

Choose a reason for hiding this comment

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

I wanted to use a public API but maybe it's an overkill (my bad since it was my suggestion :')).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So just revert to 46753b9 and dynamically get the dict file using jieba.DEFAULT_DICT_NAME is enough, I think.

Copy link
Contributor Author

@Snoopy1866 Snoopy1866 Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why use inspect here? It's a very heavy package, we can just use .__file__?

Should I modify the code to meet it?

Can we have this import in the function it is being used just to save a bit of import time (unless this is already imported by some other dependency)?

May be that's a compromise, but I already have no better idea about how to make a balance between using public API and avoiding importing a heavy package.

Copy link
Member

Choose a reason for hiding this comment

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

Let's just use the dunder attribute directly. Sorry for the back and forth

Copy link
Contributor Author

@Snoopy1866 Snoopy1866 Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Let's just use the dunder attribute directly. Sorry for the back and forth

Done! Please review it again.

import os
import re

Expand Down Expand Up @@ -234,7 +235,10 @@ def __init__(self, options: dict[str, str]) -> None:

def init(self, options: dict[str, str]) -> None:
if JIEBA:
dict_path = options.get('dict')
default_dict_path = os.path.join(
os.path.dirname(inspect.getfile(jieba)), jieba.DEFAULT_DICT_NAME
)
dict_path = options.get('dict', default_dict_path)
if dict_path and os.path.isfile(dict_path):
jieba.load_userdict(dict_path)

Expand Down
Loading