Skip to content

Non-mutating API for specifying macros? #2189

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

Closed
bitjson opened this issue Jul 15, 2019 · 5 comments · Fixed by #2785
Closed

Non-mutating API for specifying macros? #2189

bitjson opened this issue Jul 15, 2019 · 5 comments · Fixed by #2785

Comments

@bitjson
Copy link
Contributor

bitjson commented Jul 15, 2019

Hi all, I love the consistency of AVA’s API (especially the lack of magically-present globals).

Would the project be open to re-evaluating how macros are defined? It’s the only thing I consistently refer back to the documentation to use rather than discovering the type via inference/hover/autocomplete. It would also be nice to avoid mutating a Macro object to define test titles (when using tslint-immutable or similar).

So rather than:

import test, { Macro } from 'ava';

const macro: Macro<[string, number]> = (t, input, expected) => {
	t.is(eval(input), expected);
}

// tslint:disable-next-line: no-object-mutation
macro.title = (providedTitle = '', input, expected) => `${providedTitle} ${input} = ${expected}`.trim();

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

Maybe something like:

import test, { Macro } from 'ava';

const macro: Macro<[string, number]> = {
    macro: (t, input, expected) => {
	t.is(eval(input), expected);
    },
    title: (providedTitle = '', input, expected) => `${providedTitle} ${input} = ${expected}`.trim();
} 

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

I think this might also make the macro API a little easier to discover by type inference – you'd start getting autocomplete assistance immediately after providing an object to the test method, e.g.:
test({ [autocomplete offers "macro" and "title" here] }).

Thoughts? If there’s interest, I can try to send a non-breaking PR (where the mutating API is still supported).

@novemberborn
Copy link
Member

I've found the TypeScript syntax quite cumbersome. We could do test.macro() which could create a macro, and have that as an import, too:

const noTitle = test.macro((t, input: string, expected: number) => {})

const withTitle = test.macro({
  title: () => 'title',
  fn (t, input: string, expected: number) {}
}

What do you think?

@bitjson
Copy link
Contributor Author

bitjson commented Jul 16, 2019

I think that would be a major improvement! I especially like that the types of all parameters can be inferred by the time we hit the first (.

For the withTitle example, maybe the title function could just be an optional second argument to keep it simple?

It would also be nice to make the Macro type clearer by using multiple generic parameters rather than the current ArrayLike wrapper, e.g. Macro<[string, number]>. With some clever overloading of the first generic parameter, we could also maintain backward compatibility and let people upgrade over time.

That would look something like:

// withTitle: Macro<string, number>
const withTitle = test.macro<string, number>(
  (t, input, expected) => {},
  (providedTitle = '', input, expected) => 'title';
);

So our example from the documentation is down to:

import test from 'ava';

const withTitle = test.macro( // TypeScript: test.macro<string, number>(...
  (t, input, expected) => {
    t.is(eval(input), expected);
  },
  (providedTitle = '', input, expected) => `${providedTitle} ${input} = ${expected}`.trim();
);

test(macro, '2 + 2', 4); // title: "2 + 2 = 4"
test(macro, '2 * 3', 6); // title: "2 * 3 = 6"
test('providedTitle', macro, '3 * 3', 9); // title: "providedTitle 3 * 3 = 9"

@novemberborn
Copy link
Member

For the withTitle example, maybe the title function could just be an optional second argument to keep it simple?

I don't think passing two functions will be clear, no matter how much your editor may use the type definition to guide you in the right direction. We could make the second argument an object with a title property so at least specifying the implementation stays the same.

It would also be nice to make the Macro type clearer by using multiple generic parameters rather than the current ArrayLike wrapper, e.g. Macro<[string, number]>. With some clever overloading of the first generic parameter, we could also maintain backward compatibility and let people upgrade over time.

That would look something like:

// withTitle: Macro<string, number>
const withTitle = test.macro<string, number>(
  (t, input, expected) => {},
  (providedTitle = '', input, expected) => 'title';
);

TypeScript lets you do that, with arbitrary arguments?

@bitjson
Copy link
Contributor Author

bitjson commented Jul 16, 2019

We could make the second argument an object with a title property so at least specifying the implementation stays the same.

You're right, that sounds great to me. 👍

with arbitrary arguments

Actually, I didn't realize until just now that arbitrary arguments was intentional. 😅 That definitely makes sense, maybe it would be good to add a note to the documentation? I assumed macros always had an input and an expected parameter, and I didn't realize you could add arbitrary other parameters.

@novemberborn
Copy link
Member

You're right, that sounds great to me. 👍

Cool. Are you interested in taking this on?

with arbitrary arguments

Actually, I didn't realize until just now that arbitrary arguments was intentional.

To clarify, I think this means the generic type has to be an array.

maybe it would be good to add a note to the documentation?

Yea could say something like "you can pass any arguments". Would you like to do a PR for that? 😄

@novemberborn novemberborn self-assigned this May 24, 2020
@novemberborn novemberborn added this to the 4.0 milestone Jun 6, 2021
novemberborn added a commit that referenced this issue Jul 5, 2021
Fixes #2189. `test.macro()` returns an object that can be used with
`test()` and hooks. The `t.context` type is inherited from `test`.

Like with AVA 3, regular functions that also have a `title` property
that is a string-returning function are supported. However this is no
longer in the type definition.

Instead the recommended approach is to use `test.macro()` to declare
macros. At a TypeScript level these are easier to discriminate from
regular implementations.
novemberborn added a commit that referenced this issue Jul 10, 2021
Fixes #2189. `test.macro()` returns an object that can be used with
`test()` and hooks. The `t.context` type is inherited from `test`.

Like with AVA 3, regular functions that also have a `title` property
that is a string-returning function are supported. However this is no
longer in the type definition.

Instead the recommended approach is to use `test.macro()` to declare
macros. At a TypeScript level these are easier to discriminate from
regular implementations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants