Skip to content

Use doc_auto_cfg#1763

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
tcharding:02-29-doc-auto-cfg
Mar 30, 2023
Merged

Use doc_auto_cfg#1763
apoelstra merged 1 commit intorust-bitcoin:masterfrom
tcharding:02-29-doc-auto-cfg

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Mar 29, 2023 •
edited by apoelstra
Loading

If we use #![cfg_attr(docsrs, feature(doc_auto_cfg))] instead of #![cfg_attr(docsrs, feature(doc_cfg))] we no longer need to manually mark types with #[cfg_attr(docsrs, doc(cfg(feature = "std")))].

Sweeeeeet.

Props to pezcore for the lesson :)

@tcharding
Use doc_auto_cfg
a189942 
If we use `#![cfg_attr(docsrs, feature(doc_auto_cfg))]` instead of
`#![cfg_attr(docsrs, feature(doc_cfg))]` we no longer need to manually
mark types with `#[cfg_attr(docsrs, doc(cfg(feature = "std")))]`.

Sweeeeeet.
@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Mar 29, 2023

I'm not really sure, if we do this we will need to check if any odd cfgs (like rust_v_x) get rendered correctly. IIRC there were some issues with this. If this renders correctly today then I guess I'm fine with this.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Mar 29, 2023 •
edited
Loading

From my investigation just now it looks like this PR actually improves the situation.

Build docs on master

impl<'a> From<&'a Script> for Arc<Script>
Available on target_has_atomic="ptr" only.

Build docs on this branch

impl<'a> From<&'a Script> for Arc<Script>
Available on non-rust_v_1_60 or target_has_atomic="ptr only.

(Did not check other instances so there may be regressions elsewhere. I only checked this one and Bound (in script/borrowed.rs), and found that we forgot to mark the delegate_index! call with the correct attribute.)

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Mar 29, 2023

If this renders correctly today then I guess I'm fine with this.

I'm surprised you are not more enthusiastic for this change, 100 lines of red in the diff coupled with less maintenance and never having to remember to add the additional attribute - seems like a big win to me.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Mar 29, 2023

Note to self, if this merges we need to change #1127 to remove the attribute from CONTRIBUTING.md.

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Mar 29, 2023

Nice! I also suspected that it would improve the situation somehow, because surely we couldn't have gotten it all correct doing things by hand ... am glad to be proven correct.

apoelstra
apoelstra approved these changes Mar 30, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK a189942

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Mar 30, 2023

Cool, you've convinced me.

Kixunil
Kixunil approved these changes Mar 30, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK a189942

@apoelstra apoelstra merged commit 895db71 into rust-bitcoin:master Mar 30, 2023
@tcharding tcharding deleted the 02-29-doc-auto-cfg branch March 30, 2023 22:07
@Kixunil Kixunil mentioned this pull request Mar 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tcharding @Kixunil @apoelstra