Skip to content

Conversation

ChristianMurphy
Copy link
Contributor

This brings support for test macros in the typescript.
It adds a macro interface, overloads the test function definition, and overloads all combinations of test modifiers.

Similar to #895 but built on top of #884

@sindresorhus
Copy link
Member

// @ivogabe @SamVerschueren

@SamVerschueren
Copy link
Contributor

@ChristianMurphy I'll try looking into it this evening or tomorrow. If not, don't hesitate to ping me back in a couple of days, quite busy lately :).

types/make.js Outdated
output += '\t' + writeFunction(part, 'name: string', 'void');
} else {
const type = testType(parts);
output += '\t' + writeFunction(part + '<I, E>', 'implementation: Macro<I, E, ' + type + 'Context>, input: I, expected: E');
Copy link
Contributor

@ivogabe ivogabe Jul 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't allow multiple macro's (like in the test function in base.d.ts). I would define a type alias, after the definition of Macro in base.d.ts:

type Macros<I, E, T> = Macro<I, E, T> | Macro<I, E, T>[];

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put the macro declarations after the original declarations? Editors will then first show the macro-less declaration, and that is probably the most used one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and Done

@ChristianMurphy ChristianMurphy force-pushed the add-test-macro-definition branch from 79a5080 to 2beeebb Compare July 24, 2016 14:05
@ivogabe
Copy link
Contributor

ivogabe commented Jul 25, 2016

Looks good to me now. @SamVerschueren have you taken a look at this?

@ChristianMurphy
Copy link
Contributor Author

// @SamVerschueren

types/base.d.ts Outdated
context: any;
}

export interface Macro<I, E, T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I would put the T argument at the first position. Doesn't it make more sense? Because the macro also provides the testcontext as the first argument.

export interface<T, I, E>

Same for Macros

// @ivogabe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with T as first type param

@SamVerschueren
Copy link
Contributor

Sorry for the late reply guys. Only have one comment. If that's answered/addressed, seems good to merge.

types/base.d.ts Outdated

export interface Macro<T, I, E> {
(t: T, input: I, expected: E): void;
title? (providedTitle: string, input: I, expected: E): string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last small remark. Should we call this title: string instead of providedTitle: string? Autocompletion tools show the param names and title: string might be better.

// @ivogabe @sindresorhus

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming it to title might be confusing, since you'd then have a function and an argument both named title. I did a quick test and autocompletion doesn't show the argument name when implementing it like this:

export interface Macro<T, I, E> {
    (t: T, input: I, expected: E): void;
    title? (providedTitle: string, input: I, expected: E): string;
}

const macro: Macro<...> = () => {};
macro.title = (/* cursor */) => "";

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call this title: string instead of providedTitle: string?

I would agree with @ivogabe I would lean against renaming, it could be confusing. Additionally providedTitle is how this is documented in the readme.

I did a quick test and autocompletion doesn't show the argument name when implementing it like this

I've noticed autocomplete is not suggesting as well, though the type checker is validating correctly.
I'm open to suggestions on how to improve the typing to get autocompletion here.

types/base.d.ts Outdated
export interface Macro<T, I, E> {
(t: T, input: I, expected: E): void;
title? (providedTitle: string, input: I, expected: E): string;
(t: T, input?: I, expected?: E): void;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should not be marked optional here, since that would require a user to make them optional as well. Making them optional in test and the generator script is sufficient. (A function with less arguments is assignable to a function type with more arguments.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, updated

@sindresorhus
Copy link
Member

Is this good to merge now?

@SamVerschueren
Copy link
Contributor

SamVerschueren commented Aug 5, 2016

I'm not entirely sure to be honest. It doesn't feel 100% right to me as a macro could actually be anything.

This implementation was based upon the example in the readme.

function macro(t, input, expected) {
    t.is(eval(input), expected);
}

test('2 + 2 === 4', macro, '2 + 2', 4);
test('2 * 3 === 6', macro, '2 * 3', 6);

But what if I want to do this

function macro(t, obj) {
    for (const key of Object.keys(obj)) {
        t.is(eval(obj[key]), key);
    }
}

test('2 + 2 === 4', macro, {4: '2 + 2'});
test('2 * 3 === 6', macro, {6: '2 * 3'});

This doesn't follow the input, expected order as defined by the typescript definition. And because you can actually pass as many arguments to the macro function as you want, I would suggest adding a couple extra like this

export function test<I, E> (name: string, run: Macros<ContextualTestContext>, arg1?: any): void;
export function test<I, E> (name: string, run: Macros<ContextualTestContext>, arg1?: any, arg2?: any): void;
export function test<I, E> (name: string, run: Macros<ContextualTestContext>, arg1?: any, arg2?: any, arg3?: any): void;
export function test<I, E> (name: string, run: Macros<ContextualTestContext>, arg1?: any, arg2?: any, arg3?: any, arg4?: any): void;
export function test<I, E> (name: string, run: Macros<ContextualTestContext>, arg1?: any, arg2?: any, arg3?: any, arg4?: any, arg5?: any): void;

The current implementation forces the user to use it like the example provided in the readme. But macros are much more powerful then that.

This is just my opinion. If you guys are fine with what we have now, It's ok to merge. Just wanted to bring this up to everyones attention.

@sindresorhus
Copy link
Member

test('2 + 2 === 4', macro, {4: '2 + 2'}); should definitely be supported.

@ivogabe
Copy link
Contributor

ivogabe commented Aug 5, 2016

@SamVerschueren Then all parameters are still typed as any, so that doesn't give any type safety. Using ...args: any[] would then even be better, as it allows an arbitrary amount of arguments. Your example will work. When you give t and obj type annotations, you will get this macro: Macro<ContextualTestContext, { [ key: number ]: string }, {}>. The last position isn't used and defaults to {}. So, macros with less than two arguments work; macros with more than two parameters do not work.


Maybe a bit off-topic, but I think that it might be better to define macros based on partial application/currying like this: (using the same example)

const macro = (obj: { [key: number]: string }) => (t: ContextualTestContext) => {
    for (const key of Object.keys(obj)) {
        t.is(eval(obj[key]), key);
    }
}
test('2 + 2 === 4', macro({4: '2 + 2'}));
test('2 * 3 === 6', macro({6: '2 * 3'}));

That doesn't require a special overload for macros, and can thus be typed correctly. It might be an idea to type the macro overload with ...args: any[] and advise TypeScript users to use currying like in the previous example.

@sindresorhus
Copy link
Member

Alright, let's go with ...args: any[] then. We can improve it when microsoft/TypeScript#5453 is fixed.

but I think that it might be better to define macros based on partial application/currying like this: (using the same example)

👍 Could be added to the TypeScript recipe.

@ChristianMurphy
Copy link
Contributor Author

Updated with ...args: any[]

@sindresorhus sindresorhus merged commit 8816faf into avajs:master Aug 11, 2016
@sindresorhus
Copy link
Member

Thank you @ChristianMurphy for this excellent contribution, and to @ivogabe and @SamVerschueren for reviewing :)

@ChristianMurphy ChristianMurphy deleted the add-test-macro-definition branch August 11, 2016 13:43
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.

5 participants