Support for generation of documentation comments for bindings #1498
Conversation
Add initial version of Function::from_str() (docs)
now it is! |
| /// | ||
| /// Rust doc comments are silently converted (during parsing) to attributes of form: | ||
| /// #[doc = "documentation comment content"] | ||
| fn extract_doc_comment(attrs: &[Attribute]) -> Option<String> { |
There was a problem hiding this comment.
I'm guessing that since this review thread has not received new comments, nothing has changed in this regard?
|
Note that I just reviewed #1493 and personally favor that approach. UniFFI folks have a meeting tomorrow anyway, so I'll bring it up there. |
@badboy please notice that there is a PR entering this one, which includes #1493 , so we could include both approaches with this PR. Moreover, I don't understand favoring that approach. It forces developers to use UDL, while the proc-macros approach is clearly a step towards NOT using UDL. Meaning that if I want some comments on a structure, I have to write down the UDL spec, while the Rust implementation will always be there. It also precludes from various other tools which may be developed for Rust, having quite a larger community of developers than the one of UDL. It's also more natural to write comments on Rust code (using |
Same question here too. It's great to have both as options in my opinion. We don't have a UDL file in our project, would prefer to keep it that way to avoid duplication, and want the Rust library to be the canonical single source of documentation. This PR accomplishes the doc goal along with the others, while the other one is only really useful for those already committed to UDL. |
Sorry for the miscommunication there. That's on me. |
|
@badboy Please note that there is still a major unresolved thread here: #1498 (comment) |
@jplatte is there a way we can help? I'm also unsure whether I fully understand the issue: What I got is that it is a problem that the uniffi docs is a separate crate, because you would like to parse both proc macros and documentation in one pass - but isn't it better for them to be kept separated, since they deal with totally different issues? We could integrate it, but won't it be more of a burden to maintain? Also, I'd expect documentation parsing to be much quicker than expansion of proc macros. Is the performance really going to be an issue here? Let's also take into consideration that docs generation is easy to switch on and off. |
|
As I understand there are 3 use cases when using docstrings.
I find 3. to be quite an interesting use case. Somebody had mentioned before that this might be a preferred method for some, since documentation in Rust code may be more up to date than in UDL files. Supporting this use case is only possible with changes implemented in this PR. Since proc macros are not used, the only way to extract documentation from Rust code is to parse the Rust code in a separate pass. I can definitely imagine how this would be useful in some projects. From brief comments by @jplatte, supporting 2. should be trivial with changes from my PR. Some changes are required to extract doctsring from @MeerKatDev Which use case do you prefer? |
|
I would expect doc collection to be a regular part of processing both UDL & the proc-macro input. That would also mean that mixing UDL and proc-macros is supported for docs in exactly the same ways it is for bindings generation. I have noticed that this PR diverges from that, but I don't understand the details of how it actually works other than that it processes Rust files with |
|
One possibly noteworthy feature that proc-macros support naturally is target-specific code (e.g. |
|
@arg0d This PR was written with the 3. use case in mind, when still the 2. wasn't available. I - and I think many other developers - will prefer to write the documentation in Rust, so I prefer 3. (is it what you were asking?) @jplatte that would require a much tighter integration though, and additional (mandatory) maintenance on the way, from your side. As we understood, this had to end up as a modular library, and not everyone will want to bother with documentation. I think that this reasoning would be enough to lean on the side of making separate passes. |
|
I think uniffi could support all 3 use cases. The only problem I can imagine is lack of tests to cover all 3 use cases for docstrings. Assuming there are enough tests, supporting all 3 use case seems quite trivial. There isn't mutable state to worry about, its just moving strings from A to B. I don't see a problem with conditional compilation, assuming 2nd option is fully supported. When using proc-macros, conditional compilation would work fine by using proc macros to extract docstrings. When using this 3rd option to extract docstrings from Rust code, conditional compilation wouldn't work correctly, but I don't see that as a big problem, especially if its clearly documented somewhere how the documentation is extract with this mode. I think it would be most appropriate to do all this over the course of multiple PRs. The 1st option is covered by #1493. The 2nd is not currently covered, but it can be covered in future PR. The 3rd option is covered by this PR. I think we could merge both of these PRs sequentially. I'm not very particular about the order in which PRs are merged. I just think merging either one of these PRs is going to get the the process moving again. The only difference between these is naming in Rust code / templates. #1493 uses |
Yes, this is a PR to the main UniFFI repo afterall, not a separate one.
I'm not clear on why this would be more complicated maintenance-wise
I don't think anybody would be forced to write documentation comments with the alternative solution? Also, it might still be worth it to generate documentation without doc comments, just so the structure of the generated bindings is clearer. All of this is just my personal opinion as a contributor / collaborator though, I'm not a maintainer; this needs input from somebody from the actual team maintaining this. |
|
@badboy Hi, I saw you guys added support for the UDL docstrings, is this PR being considered for merging too? |
We're discussing it tomorrow, at which point I'll get back to you. |
I suspect #1493 being merged will have a significant impact on this PR, right? Eg, On one hand I don't want to ask you to do work which might not get accepted, but OTOH this patch now looks much bigger than it probably needs to be, which makes it difficult to see the bigger picture |
There's a PR on the other repo that does merge in the code from 1493, thus making this a bit smaller overall. It would still need a rebase. (I also had some other comments on the code that I did not yet submit) |
|
Some of us maintainers discussed this last week. We do not think that having a third way, to write docs in Rust but have all definitions in UDL, is beneficial. Therefore I will close this PR. Side note: There's a couple of details in this PR that would need changes anyway, e.g. the markdown parsing is not something we would do as-is. As implemented here it will break on real-world docs and relies on a documentation convention that is not adhered to by all Rust code (nor properly documented). We could discuss how to tackle that issue separately though. |
We've developed a solution for attaching documentation comments for generated bindings.
Doc-comment are extracted from accompanying intermediary Rust code, attached to
ComponentInterfaceand then attached to bindings in templates.See example.
Related issue: #200