Skip to content

Documentation for proc macros not rendered correctly #904

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
Lorak-mmk opened this issue Jan 4, 2024 · 5 comments · Fixed by #907
Closed

Documentation for proc macros not rendered correctly #904

Lorak-mmk opened this issue Jan 4, 2024 · 5 comments · Fixed by #907
Assignees

Comments

@Lorak-mmk
Copy link
Collaborator

Documentation for SerializeCql and SerializeRow is not rendered properly.
https://docs.rs/scylla/latest/scylla/derive.SerializeCql.html
https://docs.rs/scylla/latest/scylla/derive.SerializeCql.html

Both docs say See the documentation for this item in the scylla crate. - which we are obviously looking at.

I'm not sure why that happens. scylla re-exports those items from scylla-cql crate, which re-exports them from scylla-macros and documents them.
Documentation for scylla-cql renders correctly, so I don't get why scylla uses docstrings directly from scylla-macros. @piodul any ideas?

@piodul
Copy link
Collaborator

piodul commented Jan 4, 2024

I'm not sure either. Perhaps the behavior of cargo doc changed in some version.

The reason why the "proper" documentation for those macros is in scylla-cql (the docstrings are actually there, not in scylla) is because the code examples need stuff from scylla-cql, so putting them in scylla-macros does not work out of the box.

We should check if there is a way to put the docstrings into scylla-macros and make them compile. If that fails, we should see whether we can control the behavior of cargo doc and tell it to use the docstring from scylla-cql.

@Lorak-mmk
Copy link
Collaborator Author

I tried to get the right documentation to render in scylla without moving the comments, without success. No combination of #[doc(inline)], #[doc(no_inline)] and changing exports (e.g. importing SerializeRow / SerializeCql directly apart from the wildcard import) did the trick.

Do we need those examples to compile? We have other tests for those macros, so imo the easiest solution is to move the docs to scylla_macros crate and mark examples as ignore. This would get rid of another problem that I don't think can be solved differently: even in scylla_cql crate, the placeholder comment from scylla_macros is appended to docs. So the end of SerializeCql docs is currently:

#[scylla(rename = "name_in_the_udt")]

Serializes the field to the UDT struct field with given name instead of its Rust name. See the documentation for this item in the scylla crate.

Note the last sentence, that should not be there.

@piodul
Copy link
Collaborator

piodul commented Jan 5, 2024

Do we need those examples to compile? We have other tests for those macros, so imo the easiest solution is to move the docs to scylla_macros crate and mark examples as ignore.

It's not about testing the implementation, it's about making sure that examples in the documentation are up-to-date. I'd rather avoid marking them as "ignore" so that we don't accidentally suggest incorrect code to the users.

How do other projects handle this issue? Tokio, for example - they have the tokio::main macro which is defined in the tokio-macros crate but re-exported in tokio.

I now remember that I investigated this long ago actually. I wrote a proposal for scylla-udf (scylladb/scylla-rust-udf#14) which also has the same problem. There, the suggestion was to make scylla-udf-macros dev-depend on scylla-udf. This probably means that scylla-udf should re-export the macros optionally, only if "macros" crate feature is enabled - so that the dev-dependency doesn't introduce a cycle.

In our case, scylla-macros could dev-depend on scylla-cql. Of course, this would mean that we can't use procedural macros from scylla-macros in scylla-cql, but that is probably ok (scylla does use some macros, but it should be fine).

@Lorak-mmk
Copy link
Collaborator Author

Adding dev-dependency on scylla in scylla-macros allows doc tests to compile. However, it creates another problem :( Documentation for serialization macros refers to items from scylla_cql - and I don't see a good way to do this, even if scylla_cql is a dev-dependency.

One option is to use absolute links to docs.rs - it is a problem for very obvious reasons.
Tokio decided to use relative links. In our case it would look like this:
/// Derive macro for the [SerializeCql](../scylla_cql/types/serialize/value/trait.SerializeCql.html) trait
but this makes this link only work in scylla-cql crate when uploaded to docs.rs
Notice how the links to Runtime and Builder don't work in tokio-macros crate docs: https://docs.rs/tokio-macros/latest/tokio_macros/attr.main.html

That could maybe be a good tradeoff, but I don't think we could get the links working both in scylla and scylla_cql - unless we can somehow get scylla_cql on the Crates list on the left here: https://docs.rs/scylla/latest/scylla/ . Do you know how to do it?

@piodul
Copy link
Collaborator

piodul commented Jan 8, 2024

That could maybe be a good tradeoff, but I don't think we could get the links working both in scylla and scylla_cql - unless we can somehow get scylla_cql on the Crates list on the left here: https://docs.rs/scylla/latest/scylla/ . Do you know how to do it?

No, I don't. However, users should look at the docs in the scylla crate most of the time. Until there is support in cargo doc for links like this (see rust-lang/rust#74481), I think we can live with this approach.

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 a pull request may close this issue.

2 participants