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

fix search word splitter in JavaScript #3154

Merged
merged 1 commit into from
Nov 19, 2016

Conversation

shibukawa
Copy link
Contributor

@shibukawa shibukawa commented Nov 16, 2016

Now I made code generator and it works as same as Python.
It fix #3150

@shibukawa shibukawa changed the title fix #3150 fix search word splitter in JavaScript Nov 16, 2016
@shibukawa shibukawa force-pushed the fix/jswordsplitter branch 2 times, most recently from 3a5e061 to 9b19531 Compare November 16, 2016 03:37
~~~~~~~~~~~~~~~~~~~~~~~~

Provides Python compatible word splitter to JavaScript

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment like "DO NOT EDIT. This is generated by compat_regexp_generator.py."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,128 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Should this script be installed? What about move this into utils directory of root directory?

Copy link
Member

Choose a reason for hiding this comment

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

compat_regexp is very vague for me.
What about jssplitter_generator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@tk0miya
Copy link
Member

tk0miya commented Nov 17, 2016

LGTM with nits

@tk0miya
Copy link
Member

tk0miya commented Nov 18, 2016

LGTM!

@@ -10,6 +10,7 @@ Bugs fixed
* #3068: Allow the '=' character in the -D option of sphinx-build.py
* #3074: ``add_source_parser()`` crashes in debug mode
* #3135: ``sphinx.ext.autodoc`` crashes with plain Callable
* #3150: Fix query word splitter in JavaScript. It behaves as same as Python's regular expressio.
Copy link
Member

Choose a reason for hiding this comment

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

"It behaves the same as Python's regular expression."

console.log("test splitting Emoji(surrogate pair) characters. It should keep emojis.")
assert.deepEqual(['😁😁'], splitQuery('😁😁'));
console.log(' ... ok\\n')

Copy link
Member

Choose a reason for hiding this comment

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

It'd be great to add a test with umlauts here, like:

console.log("test splitting umlauts. It should keep umlauts.")
assert.deepEqual(['Löschen', 'Prüfung', 'Abändern', 'ærlig', 'spørsmål'], splitQuery('Löschen Prüfung Abändern ærlig spørsmål'));
console.log('   ... ok\\n')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added. Thank you for your feedback.

@shimizukawa
Copy link
Member

LGTM!

@tk0miya
Copy link
Member

tk0miya commented Nov 19, 2016

Okay, I will merge this tomorrow.
@shibukawa could you update this PR until tomorrow?
If impossible, I will update CHANGES entry and please send us another PR to update test for umlauts.

@shibukawa shibukawa force-pushed the fix/jswordsplitter branch 3 times, most recently from 412e1de to 873fab5 Compare November 19, 2016 14:51
@tk0miya tk0miya merged commit b329fab into sphinx-doc:stable Nov 19, 2016
@tk0miya
Copy link
Member

tk0miya commented Nov 19, 2016

Merged. Thank you always!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants