Skip to content

Conversation

@antalsz
Copy link
Contributor

@antalsz antalsz commented Jun 28, 2024

Closes #50

@antalsz antalsz requested a review from Shadow53 June 28, 2024 23:08
@antalsz antalsz force-pushed the 50-generate-documentation branch from 6e1436a to 3480712 Compare July 1, 2024 17:35
@antalsz
Copy link
Contributor Author

antalsz commented Jul 1, 2024

I've added this documentation, which is great, but it occurs to me that there's another way: have the enum wrappers take a visibility argument and not automatically generate pub output. Then documentation wouldn't be necessary. Now that I've done this work it's probably still worth including it, but maybe we should do that as well? That would be a breaking change, though.

@antalsz antalsz force-pushed the 50-generate-documentation branch from 3480712 to a2fa204 Compare July 1, 2024 17:51
@antalsz
Copy link
Contributor Author

antalsz commented Jul 1, 2024

Also, I feel like this PR ought to come with a test but I'm not sure how to test this. I could add something that turns on #[deny(missing_docs)] and see how compilation goes, but I feel like we want to make sure the specific output is correct, too – maybe with some sort of snapshot test?

@antalsz antalsz force-pushed the 50-generate-documentation branch from 00fde47 to 974a75e Compare July 2, 2024 22:01
@antalsz
Copy link
Contributor Author

antalsz commented Jul 2, 2024

This is now available for review, but I have some concerns about the tests. Not only do they depend on eupn/macrotest#115 (currently we point to anatalsz/macrotest to get it early), but we also have to play games with the very specific OS we do the expansion for, which necessitates the end user using rustup target add. To quote my commit message in 497e4ca

We have to specify a specific OS target because until PyO3 v0.22, PyO3 transitively depends on the rust-ctor crate, which generates different output on different OSes. Once we're doing that, we have to specify a specific Python ABI so that PyO3 doesn't get alarmed about cross-compilation. This is all a minor headache.

One concern about this is that everyone testing locally has to run rustup target add x864_64-unknown-linuxgnu. CI doesn't, because I deliberately picked the triple that was running on CI, but any local development that isn't on that OS does. Is this worth it?

An alternative design would be to add a post-processing filter to macrotest, and then specify (the moral equivalent of) grep -v link_section as the postprocessor, but that's another extension on top of macrotest, and less obviously one that the maintainers thereof would be interested in.

@antalsz
Copy link
Contributor Author

antalsz commented Jul 9, 2024

I've now split out the question of testing the macro expansion into #53 (see issue #52). Due to the complexity of testing OS-specific macro expansion, I now believe we should merge this and rely on manual inspection of the generated output until we can figure out how best to implement #53.

This enables easier manual testing with `cargo expand`, and will enable easier
snapshot testing later.
@antalsz antalsz force-pushed the 50-generate-documentation branch from 627e224 to f3ca274 Compare July 9, 2024 00:32
@antalsz antalsz merged commit c03ebca into main Jul 12, 2024
antalsz added a commit that referenced this pull request Jul 12, 2024
Force Knope to pick up c03ebca, the merge of #51
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.

Document all generated methods

3 participants