New feature: Typescript compiler #155
Conversation
| @@ -647,7 +685,7 @@ check_go_%: $(STEMMING_DATA_ABS)/% | |||
| fi | |||
| @rm tmp.txt | |||
|
|
|||
| export NODE_PATH = $(js_runtime_dir):$(js_output_dir) | |||
| export NODE_PATH = $(js_runtime_dir):$(js_output_dir):$(ts_runtime_dir):$(ts_output_dir) | |||
There was a problem hiding this comment.
It would be cleaner to arrange to set this to just the paths that are relevant depending on whether we're doing javascript or typescript - then we don't have to worry that we might be pulling parts of the other one when running tests.
There was a problem hiding this comment.
This is all Greek to me. I just did some guessing :)
| import * as fs from 'fs'; | ||
|
|
||
| function usage() { | ||
| console.log("usage: stemwords.js [-l <language>] -i <input file> -o <output file> [-c <character encoding>] [-p[2]] [-h]\n"); |
| if (!lc_name.match('\\W') && lc_name != 'base') { | ||
| let algo = lc_name.substr(0, 1).toUpperCase() + lc_name.substr(1); | ||
| try { | ||
| const stemmer = require(lc_name + '-stemmer.js'); |
There was a problem hiding this comment.
.js here too ... which presumably means we're testing the JS stemmers, not the typescript ones!
| @@ -0,0 +1,1308 @@ | |||
|
|
|||
There was a problem hiding this comment.
This file is very similar to generator_js.c - from a maintenance point of view I think it would make more sense to share a single generator implementation between javascript and typescript and just parameterise the small number of differences - that way future fixes only need applying in one place rather than two (if there are two places to fix, inevitably one gets missed sometimes).
Also, it seems many of the differences are due to trying to format the generated code to match typescript code formatting conventions. I'm not sure how worthwhile a goal that is - having generated code with a basically readable formatting to aid debugging generator problems is useful, but coding conventions almost universally aim at making code more maintainable, and the generated code inherently doesn't need to be maintained directly.
So I'd be inclined to just tell typescript to shut up about code formatting (with more/broader eslint-disable magic comments), but if that's not possible we could format the javascript output in the same way.
There's also the approach the Go generator takes which is to run the snowball .go outputs through gofmt to mechanically reformat them to match Go coding conventions. That has the advantage that there's no snowball-specific work to do if new formatting checks get added.
There was a problem hiding this comment.
You're absolutely right on all points. Not sure if and when I will have time to do this though.
Magic comments would do, for sure. I believe the most important parts are:
- Modern Typescript class syntax is preferred:
export class SwedishStemmer extends BaseStemmer - Prefix all members and methods with
this.
I'm no Javascript expert, but once you start mixing require() with some legacy libraries and the modern import syntax, the compiler doesn't seem very happy. That's the reason I created this PR in the first place.
| mkdir -p $${dest} && \ | ||
| mkdir -p $${dest}/$(ts_runtime_dir) && \ | ||
| mkdir -p $${dest}/$(ts_sample_dir) && \ | ||
| cp -a doc/libstemmer_ts_README $${dest}/README.rst && \ |
There was a problem hiding this comment.
There's no doc/libstemmer_ts_README so make dist fails (hence the CI failures)
|
This PR still needs work, and #183 suggests a different approach:
Thoughts? Should we close this PR in favour of taking that approach instead? |
|
Taking that "thumbs up" as a yes, and closing. |
Derived a
Typescriptcompiler from the existingJavascriptcompiler.I believe
snowballwill greatly benefit from this. Please consider a merge after scrutinizing all my tweaks. Particularly the build scripts... I did my best trying to figure out all parameters and stuff 😄