This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[contracts] Add docs generator for the contracts API to the
#[define_env]
macro #13032[contracts] Add docs generator for the contracts API to the
#[define_env]
macro #13032Changes from 4 commits
04769ea
e7f6b78
606d58d
6cf881d
c558c5e
cd2007b
ce9031b
dced64c
98a6c63
ab36942
f8def8c
f118f74
fff947e
1af74cc
1272108
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should rename
doc
toemit_doc
. I think this makes it clearer what is happening. I won't die on that hill if you disagree, though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to me, concise variant is more elegant and could hardly be misinterpreted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But isn't it the same attribute name used for rust doc comments itself? To be that seems confusing. Also, why doesn't this PR add this attribute to the
define_env
invocation in runtime.rs?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's exactly the point. The idea is to make it short, appropriate and familiar to the user: at the end of the day, it is for generating the rustdoc. I don't see it is confusing, as here
doc
attr is used as input todefine_env(doc)
rather than solo (as when it's for a single doc comment)I was thinking to leave it off by default. But could be done vice versa, why not. Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
Most important point for me is that the standard docs.rs builds of this crate contain this documentation. Therefore it should be on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. The only objection comes to my mind is that it probably should be turned off for production use not to bloat runtime size in vain. Should we add a note to the macro docs at least?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not bloat the runtime size as it is dead code and hence removed by the compiler. At least when using lto.
But you have a point there. I think you should just add
#[cfg(doc)]
to the emittedapi_doc
module (and its re-rexports). Then this code will be compiled out for every target but rustdoc.