Use pure tokenizer (#8)

thlorenz / redeyed

Takes JavaScript code, along with a config and returns the original code with tokens wrapped and/or replaced as configured.

MIT License

25 stars 7 forks source link

Use pure tokenizer (#8) #9

Closed ariya closed 8 years ago

ariya commented 8 years ago

I'd be ok with never obtaining the ast in the first place, or at least turn that off and allow turning it on via an option.

What should be the option name and the default value? ast and false as the defaut is OK for you?

And I can update the docs accordingly.

thlorenz commented 8 years ago

I'd prefer this behavior:

ast option is false (=default):

Behavior: tokenize the input (don't even try to build the AST)
Return value: ast is not there (undefined)
Exception: only thrown if the tokenizer fails

ast option is true:

Behavior: parse the input (no fallback to tokenization)
Return value: ast is returned when the input is parsed successfully (syntactically correct)
Exception: thrown if the parser fails with message that user should try again with ast: false

ast or buildAST are both good names for that option .. I'll leave it up to you.

ariya commented 8 years ago

I'll adjust the implementation to match the proposed behavior.

Note that this new behavior (buildAst is false by default) is a breaking change. I can check and tweak cardinal (e.g. it assumes invalid code will throw an exception), but other downstream users might break.

thlorenz commented 8 years ago

Note that this new behavior (buildAst is false by default) is a breaking change

Yes I know, we'll publish a new major version.

ariya commented 8 years ago

Yes I know, we'll publish a new major version.

If you'd like a transition time (only a minor version update), we can do the following:

Implement the proposed behavior
Ensure that the default value for buildAst to true
Modify cardinal to use redeyed with buildAst = false

That way, cardinal can benefit from this pure tokenizer mode while other downstream users have some time to evaluate and migrate.

Entirely up to you, I can live with or without the transition.

thlorenz commented 8 years ago

Let's go with breaking things as I think it's better in general to just use the tokenizer by default.

ariya commented 8 years ago

Let's go with breaking things as I think it's better in general to just use the tokenizer by default.

In that case, is there a strong reason to keep the AST mode?

thlorenz commented 8 years ago

Yes, since some people may want to build tools around this that use the AST, so let's not totally kill it.

ariya commented 8 years ago

@thlorenz Any review/comment on the latest commit?

thlorenz commented 8 years ago

Great work so far, just need to figure out those last tidbits. Thanks.

ariya commented 8 years ago

@thlorenz Do the added/tweaked tests look good now?

thlorenz commented 8 years ago

LGTM @ariya when you feel this is complete feel free to squash+merge to master and ping me please so I can version and publish.

ariya commented 8 years ago

@thlorenz Landed in master. Thank you!

thlorenz commented 8 years ago

Published as v1.0.0. Major upgrade due to breaking changes. Thanks for all the great work :)