Skip to content
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

Add reasons for each doctest ignore #569

Merged
merged 7 commits into from May 14, 2021
Merged

Conversation

ShadowJonathan
Copy link
Member

@ShadowJonathan ShadowJonathan commented May 10, 2021

Fixes #541.

This adds the actual reason why some tests cannot be no_run, and one actual TODO label for ruma-events-macros.

@ShadowJonathan ShadowJonathan changed the title Add reasons for each ignore Add reasons for each doctest ignore May 10, 2021
crates/ruma-events-macros/src/lib.rs Outdated Show resolved Hide resolved
crates/ruma-serde/src/raw.rs Outdated Show resolved Hide resolved
Comment on lines 51 to 52
//// TODO: Change above (`<EventKind as Parse>::parse`) to [] after fully qualified syntax is supported:
//// https://github.com/rust-lang/rust/issues/74563
Copy link
Member

Choose a reason for hiding this comment

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

Why does this commend start with four /es?

Copy link
Member Author

Choose a reason for hiding this comment

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

To leave it out of the actual doc, its more a comment on the documentation itself.

https://doc.rust-lang.org/stable/reference/comments.html#doc-comments

   //   - Only a comment
   ///  - Outer line doc (exactly 3 slashes)
   //// - Only a comment

Copy link
Member

Choose a reason for hiding this comment

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

I understand that that makes it not a doc comment but I've never seen four slashes as "comment on docs" before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Utilizing three, imo, felt more like a "lowering" from the doc, making four then is more like "above" the doc.

@@ -27,6 +27,7 @@ mod util;
/// of the struct, this simple implementation will be generated:
///
/// ```ignore
/// # // TODO: "ignore" this doctest until it is more extensively documented. (See #541)
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand this TODO comment.. This ignore is probably fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm providing reasons as to why each doctest is ignored, it's arguably bad to leave it ignored as-is, but at least i'm providing reasons as to why its ignored, so someone in the future can pick up on it.

Copy link
Member

Choose a reason for hiding this comment

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

I get that, but I don't really understand how "more extensively" documenting something here would resolve the ignore.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's some blurbs of doctest underneath this comment, I know that writing a more "complete" doctest will resolve the run and compilation errors, but at the moment i dont know how to do it, ergo "more extensively documented"

@jplatte jplatte merged commit b616ba8 into ruma:main May 14, 2021
@ShadowJonathan ShadowJonathan deleted the with-reason branch May 14, 2021 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Change ignore code blocks to no_run
2 participants