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

Remove Regex from search #8034

Merged
merged 1 commit into from
Feb 26, 2025
Merged

Conversation

facelessuser
Copy link
Contributor

Use Re in all places in the search plugin except where Unicode properties are desired (checking for Chinese chars). Use Backrefs' bre to check for Unicode properties. To minimize any differences, explicitly use script as Backrefs uses script_extensions (or scx) by default with Is* properties.

Resolves #8032

Use Re in all places in the search plugin except where Unicode
properties are desired (checking for Chinese chars). Use Backrefs'
`bre` to check for Unicode properties. To minimize any differences,
explicitly use `script` as Backrefs uses `script_extensions` (or `scx`)
by default with `Is*` properties.
@squidfunk
Copy link
Owner

Thanks for the PR! LGTM, as it detects all Chinese characters in our documentation (debug output):

Building prefix dict from the default dictionary ...
Dumping model to file cache /var/folders/h1/2tbw4c3x4cvfrx9ctcllxz8r0000gn/T/jieba.cache
Loading model cost 0.271 seconds.
Prefix dict has been built successfully.
支持
終於
支持
中文
了
文本
被
正確
分割
並且
更
容易
找到
支持
占用内存较小的词典文件
支持繁体分词更好的词典文件

Thus, I consider this safe to merge.

@squidfunk squidfunk merged commit ab15110 into squidfunk:master Feb 26, 2025
4 checks passed
@mgorny
Copy link

mgorny commented Feb 26, 2025

Thanks!

@shaheermirzacs
Copy link

The inclusion of backrefs seems to break on Python 3.8, and backrefs has a minimum requires-python of 3.9

The output below is from a uv sync, but that shouldn't change anything

  × No solution found when resolving dependencies for split
  │ (python_full_version == '3.8.*'):
  ╰─▶ Because the requested Python version (>=3.8) does not satisfy
      Python>=3.9 and backrefs==5.8 depends on Python>=3.9, we can conclude
      that backrefs==5.8 cannot be used.
      And because only backrefs<=5.8 is available, we can conclude that
      backrefs>=5.8 cannot be used.
      And because mkdocs-material==9.6.5+insiders.4.53.15 depends on
      backrefs>=5.8 and only mkdocs-material==9.6.5+insiders.4.53.15 is
      available, we can conclude that all versions of mkdocs-material cannot
      be used.
      And because pacotests:docs depends on mkdocs-material and your
      project requires pacotests:docs, we can conclude that your project's
      requirements are unsatisfiable.

      hint: `backrefs` was requested with a pre-release marker (e.g.,
      backrefs>5.8,<6.dev0), but pre-releases weren't enabled (try:
      `--prerelease=allow`)

      hint: The `requires-python` value (>=3.8) includes Python versions
      that are not supported by your dependencies (e.g., backrefs==5.8 only
      supports >=3.9). Consider using a more restrictive `requires-python`
      value (like >=3.9).

@facelessuser
Copy link
Contributor Author

So Python 3.8 is EOL. If material wants to keep supporting 3.8, they can drop the version to the one before ~=5.8.

@facelessuser
Copy link
Contributor Author

Functionally, the version right before should still work. I fixed up tests to work in pypy in 5.8, and fixed up some deprecation warnings being generated, but there shouldn't be any functional issues.

@facelessuser
Copy link
Contributor Author

Just to be clear, I drop support for all EOL Python versions in my packages. Pymdown Extensions will also drop 3.8 in its next release.

@shaheermirzacs
Copy link

It's fine if it's EOL, but the requires-python for mkdocs-material is >=3.8. Isn't that misleading?

However, I believe my "actual" problem was that I thought I had pinned the exact version of mkdocs-material to use, but I actually used >= which is probably why I'm seeing this today in the first place. Still, if I were to use the latest version I'd seem to come across this issue.

@facelessuser
Copy link
Contributor Author

facelessuser commented Feb 26, 2025

Yep, I think its fine bumping the version down if Material definitely wants to support 3.8. I'm more stating this now so @squidfunk is aware and make a decision of either dropping 3.8 in Material or requesting an earlier version of backrefs that is 3.8 compatible.

@squidfunk
Copy link
Owner

Thanks for pointing that out, I wasn't aware of that.

We're happy to drop support for Python 3.8, but as I understand, since this might break some workflows downstream, we would need to include this change in a major release. We're not able to do a major release right now, as we need to first finish the foundational work we're currently at. and since the problem solved is not a problem that our users immediately have, as we solely improved support for PyPy, something we don't even support, I think we need to revert this change, and reopen the PR for later consideration.

@facelessuser
Copy link
Contributor Author

Let me just lower the version instead

@squidfunk
Copy link
Owner

@facelessuser wow, that was quick – thanks! This makes it much easier for us 😅 I'm sorry for the inconvenience this is causing, but we're cooking up something massive, which is why we're maintaining rather conservatively.

@facelessuser
Copy link
Contributor Author

PR is up #8037. No worries, the change is trivial.

@squidfunk
Copy link
Owner

Perfect, thanks!

@squidfunk
Copy link
Owner

squidfunk commented Feb 27, 2025

@shaheermirzacs thanks to @facelessuser, this problem should now be resolved – can you confirm?

@shaheermirzacs
Copy link

Confirmed fixed! Thank you very much @facelessuser!

@squidfunk how long does it take for the change to propagate to insiders?

@squidfunk
Copy link
Owner

Done ☺️

@mgorny
Copy link

mgorny commented Mar 5, 2025

Thanks. If I may have one more suggestion, backrefs is only used if jieba is installed — perhaps it would make sense to move the backrefs import there as well, since right now the whole plugin fails to load if backrefs aren't installed, even thought they'd never be used.

@squidfunk
Copy link
Owner

It's part of our requirements.txt, so it's guaranteed to be installed. We could also move it into the _segment_chinese function, but I don't really see the point in doing that, since it's required anyway. In the future, we'll consider making segmentation an optional dependency, but for now this should be sufficient.

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.

4 participants