-
Notifications
You must be signed in to change notification settings - Fork 0
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
EmptyStatement
#2
Conversation
@sandersn, do you think this solution/implementation fits the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! A couple more suggestions as well.
src/emit.ts
Outdated
@@ -1,7 +1,7 @@ | |||
import { Statement, Node, Expression } from './types'; | |||
|
|||
export function emit(statements: Statement[]) { | |||
return statements.map(emitStatement).join(';\n'); | |||
return statements.filter(Boolean).map(emitStatement).join(';\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would personally leave the empty statements in the output, since the output should be as close as possible to the input, but with types removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, just removed the filter imteekay/mini-typescript@6182029
src/check.ts
Outdated
@@ -13,6 +13,7 @@ import { resolve } from './bind'; | |||
const stringType: Type = { id: 'string' }; | |||
const numberType: Type = { id: 'number' }; | |||
const errorType: Type = { id: 'error' }; | |||
const emptyType: Type = { id: 'empty' }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For bonus points, see what the spec says an empty statement returns and name the type after that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the spec, it says it returns empty
. Just renamed from emptyType
to empty
imteekay/mini-typescript@6da8ef0 Lmk if this is what you meant for the refactoring.
Implement the empty statement token
EmptyStatement
sandersn/mini-typescript#14