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

EmptyStatement #2

Merged
merged 10 commits into from May 2, 2023
Merged

EmptyStatement #2

merged 10 commits into from May 2, 2023

Conversation

imteekay
Copy link
Owner

@imteekay imteekay commented Feb 25, 2023

src/types.ts Outdated Show resolved Hide resolved
src/parse.ts Outdated Show resolved Hide resolved
src/parse.ts Outdated Show resolved Hide resolved
src/test.ts Outdated Show resolved Hide resolved
src/parse.ts Outdated Show resolved Hide resolved
@imteekay imteekay changed the title Handle EmptyStatement EmptyStatement Apr 22, 2023
@imteekay imteekay requested a review from sandersn April 22, 2023 17:45
@imteekay imteekay self-assigned this Apr 22, 2023
@imteekay
Copy link
Owner Author

@sandersn, do you think this solution/implementation fits the EmptyStatement exercise?

Copy link

@sandersn sandersn left a 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');

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.

Copy link
Owner Author

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' };

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.

Copy link
Owner Author

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.

@imteekay imteekay requested a review from sandersn April 28, 2023 12:57
@imteekay imteekay mentioned this pull request Apr 28, 2023
@imteekay imteekay merged commit 63bb671 into main May 2, 2023
1 check passed
@imteekay imteekay deleted the test branch May 2, 2023 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants