retextjs / retext-spell

plugin to check spelling
https://unifiedjs.com
MIT License
71 stars 16 forks source link

Replace nodehun with hunspell-spellchecker #3

Closed localjo closed 8 years ago

localjo commented 8 years ago

This PR replaces nodehun, which doesn't work with the latest verion of node and is no longer maintained, with hunspell-spellchecker.

localjo commented 8 years ago

This is a WIP. I'm having trouble getting the vfile messages to get passed back to retext-- using this plugin results in the vfile report being blank.

A few other things to note;

localjo commented 8 years ago

One other note, I was considering using franc to make dictionary selection happen automatically, but that might be suitable for a separate PR.

wooorm commented 8 years ago

Travis fails cause of a style issue, you can check those messages by running npm test locally!

localjo commented 8 years ago

Seems like the next/queue related things in the attacher, construct and transformer functions are causing problems too, but I'm having a hard time reasoning about what's going on there.

wooorm commented 8 years ago

OK, here’s what got it to work, you’ll have to implement it though :)

diff --git a/index.js b/index.js
index 6636d6a..2e72719 100644
--- a/index.js
+++ b/index.js
@@ -26,8 +26,6 @@ var spellchecker =  new Spellchecker();
  * @param {NLCSTNode} tree - Parent node of `WordNode`s.
  * @param {VFile} file - Virtual file.
  * @param {Object} config - Configuration.
- * @param {function(Error?)} next - Invoked to continue or
- *   break the middleware.
  */
 function all(tree, file, config) {
     spellchecker.use(config.dictionary);
@@ -90,7 +88,7 @@ function attacher(retext, options) {
      * results on the parent scope.
      *
      * @param {Error?} err - Construction or loading error.
-     * @param {Hun?} hun - Dictionary instance.
+     * @param {Dictionary?} dictionary - Dictionary instance.
      */
     function construct(err, dictionary) {
         var length = queue.length;
@@ -104,6 +102,7 @@ function attacher(retext, options) {
                 queue[index][3](loadError);
             } else {
                 all.apply(null, queue[index]);
+                queue[index][3]();
             }
         }

@@ -138,7 +137,8 @@ function attacher(retext, options) {
         } else if (loadError) {
             next(loadError);
         } else {
-            all(tree, file, config, next);
+            all(tree, file, config);
+            next();
         }
     };
 }

The reason was that we’re still exposing next, but now it isn’t handled. So, now we do!

Lastly, the test section for should fail construct errors on the VFile can be dropped!

localjo commented 8 years ago

So this works, except that the vfile messages just disappear and never appear in the report when retext is done running. If I console.log() the vfile messages at the end of the all() function, the messages are all there. But they don't get passed back to retext.

codecov-io commented 8 years ago

Current coverage is 100% (diff: 100%)

No coverage report found for master at 1c1a822.

Powered by Codecov. Last update 1c1a822...ce9ee98

wooorm commented 8 years ago

How does your comment relate to the successful Travis build? Seems to work?

localjo commented 8 years ago

Yeah, looks like it might be a latent problem with a race condition in my quality-docs code. 🙈 In theory, this should be working now. I just haven't been able to try it out in a real project yet.

wooorm commented 8 years ago

Yes, this is a problem in your code, not in the retext-spell itself!

localjo commented 8 years ago

Added an ignore option to allow an array of words to be ignored to be passed in like other retext plugins.

localjo commented 8 years ago

Ah, hunspell-spellchecker does do suggestions, it's just not documented. https://github.com/GitbookIO/hunspell-spellchecker/blob/master/lib/index.js#L143

I'll add that.

localjo commented 8 years ago

Or, maybe not. The suggestions feature is really slow. https://github.com/GitbookIO/hunspell-spellchecker/issues/9

wooorm commented 8 years ago

Maybe first check if its correct, and then do suggestions? That might speed it up a bit?

localjo commented 8 years ago

Yeah, even just checking incorrect words is very slow. Took > 10 minutes to check this list of non-words;

  55:6-55:32     warning  sample-rules-override.json is mispelled.  spelling
  59:115-59:124  warning  README.md is mispelled.                   spelling
  59:135-59:137  warning  23 is mispelled.                          spelling
  59:155-59:157  warning  23 is mispelled.                          spelling
  59:165-59:167  warning  16 is mispelled.                          spelling
  71:71-71:97    warning  sample-rules-override.json is mispelled.  spelling
  72:9-72:31     warning  remark-message-control is mispelled.      spelling
  76:17-76:23    warning  retext is mispelled.                      spelling
  78:4-78:15     warning  remark-lint is mispelled.                 spelling
  78:83-78:94    warning  formatting. is mispelled.                 spelling
  79:4-79:22     warning  retext-readability is mispelled.          spelling
  79:108-79:117  warning  document. is mispelled.                   spelling
  80:4-80:19     warning  retext-simplify is mispelled.             spelling
  80:73-80:89    warning  over-complicated is mispelled.            spelling
  80:90-80:98    warning  phrases. is mispelled.                    spelling
  81:4-81:19     warning  retext-equality is mispelled.             spelling
  81:100-81:109  warning  language. is mispelled.                   spelling
  82:4-82:20     warning  retext-intensify is mispelled.            spelling
wooorm commented 8 years ago

:(

In that case, leave it out!

wooorm commented 8 years ago

I’m working on merging the PR. Nice work 👍 But please adhere to the style of a project the next time! 😬

localjo commented 8 years ago

Hey! Sorry about the things I missed. 😬 Thanks for merging!