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

Support more items in #[tests] module #441

Merged
merged 1 commit into from
Apr 29, 2021

Conversation

Sh3Rm4n
Copy link
Contributor

@Sh3Rm4n Sh3Rm4n commented Mar 31, 2021

This allows every item to be defined inside the #[tests] module,
e.g. structs consts etc. except for function.

Closes #416

"function requires `#[init]` or `#[test]` attribute",
));
untouched_tokens.push(Box::new(f));
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

This might paint us into a corner, since it means we can't add other attributes in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this case as well.
Keeping the attribute requirement for functions, but allowing everything else is ok?

With the other code change, it would mean, that we can't add any attribute to other items in the future as well, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't add a newer attribute in a backwards-compatible way under the assumption that every new attribute(-name) is already used / introduced by another crate.

Do I understand that correctly?

What about namespacing? Every attribute from defmt-test lives under defmt_test::init or defmt_test::test and we generate a use defmt_test::test implicitly. Would this be possible or even useful?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't add a newer attribute in a backwards-compatible way under the assumption that every new attribute(-name) is already used / introduced by another crate.

Yeah that's correct. It wouldn't be too hard to avoid builtin attributes, but you can use arbitrary procedural macros too, which might define their own stuff.

Every attribute from defmt-test lives under defmt_test::init or defmt_test::test and we generate a use defmt_test::test implicitly.

There's no name resolution inside the test module, so we can't insert a use, but namespacing does sound like a possible solution for attributes we add later (unfortunately requiring the user to write out a rather long path).

Item::Union(t) => untouched_tokens.push(Box::new(t)),
Item::Use(t) => untouched_tokens.push(Box::new(t)),
Item::Verbatim(t) => {
untouched_tokens.push(Box::new(t));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could use a wildcard match here and accumulate syn::Items instead of Box<dyn ToTokens>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, this is a much simpler solution. 😀
Was underestimating the cleverness of the quote! macro.

@Urhengulas
Copy link
Member

Hi @Sh3Rm4n, Thank you for submitting the PR! Can you please quickly describe how your solution solves the issue :)

This allows every item to be defined inside the `#[tests]` module,
e.g. `struct`s `const`s etc. except for function.
@japaric
Copy link
Member

japaric commented Apr 9, 2021

This looks like a good start to me but I'll let @jonas-schievink sent this to bors.

I think we could support (auxiliary) functions in a new patch version and still have some degree of extensibility by reserving some attribute names, e.g. ignore. I don't see ourselves supporting a wide gamut of attribute within defmt_test::tests in any case so if we do fail to reserve an attribute name and require a semver bump to add support a new attribute I don't think that'd be too bad since it won't happen too many times. But nothing of that needs to be addressed in this PR.

@jonas-schievink
Copy link
Contributor

It looks like as written this will reject bare fns in the test module, which should give us enough flexibility to add more custom function attributes. This seems good!

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 29, 2021

Build succeeded:

@bors bors bot merged commit 697a8e8 into knurling-rs:main Apr 29, 2021
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.

defmt-test: support other items inside the #[tests] module
4 participants