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

[knowledge] Enable No ascii chars in knowledge #2377

Closed
wants to merge 1 commit into from

Conversation

jolestar
Copy link

@jolestar jolestar commented Jan 16, 2025

Relates to

#2376

Risks

Low

Background

What does this PR do?

  • The issue was fixed by updating the regex pattern to use Unicode properties:
  • Changed from: /[^a-zA-Z0-9\s\-_./:?=&]/g
  • To: /[^\p{L}\p{N}\s\-_./:?=&]/gu
  • Added 'u' flag for Unicode support
  • Used \p{L} to match any kind of letter from any language
  • Used \p{N} to match any kind of numeric character
  • The fix allows the function to properly handle multilingual content while maintaining the original sanitization goals.

What kind of change is this?

Bug fixes (non-breaking change which fixes an issue)

Documentation changes needed?

My changes do not require a change to the project documentation.

Testing

Where should a reviewer start?

Detailed testing steps

Add a unit test.

@Copilot Copilot bot review requested due to automatic review settings January 16, 2025 11:39

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hi @jolestar! Welcome to the elizaOS community. Thanks for submitting your first pull request; your efforts are helping us accelerate towards AGI. We'll review it shortly. You are now an elizaOS contributor!

@wtfsayo
Copy link
Member

wtfsayo commented Jan 16, 2025

please do it via env flag

@wtfsayo wtfsayo closed this Jan 17, 2025
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