8000 Remove no_run by tcharding · Pull Request #1430 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove no_run #1430

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 2, 2022
Merged

Remove no_run #1430

merged 1 commit into from
Dec 2, 2022

Conversation

tcharding
Copy link
Member
@tcharding tcharding commented Dec 1, 2022 •
edited by apoelstra
Loading

no_run is not needed since we already mark this up as bash which rustc doesn't run when running examples.

While the keyword bash is not currently supported it may well be in the future and since only the rust keyword causes code to run any other string is effectively a wildcard, no_run is therefore meaningful only as a convention. Lets keep bash in case support is added later on.

cc sr-gi

@tcharding
Remove no_run
13d94cb 
`no_run` is not needed since we already mark this up as `bash` which
rustc doesn't run when running examples.

While the keyword `bash` is not currently supported it may well be in
the future and since only the `rust` keyword causes code to run any
other string is effectively a wildcard, `no_run` is therefore meaningful
only as a convention. Lets keep `bash` in case support is added later on.
@tcharding tcharding mentioned this pull request Dec 1, 2022
Merged
@Kixunil
Copy link
Collaborator
Kixunil commented Dec 1, 2022

Note that no_run isn't just convention. When specified alone (or I think with rust) the code will be compiled as Rust to check that it compiles but won't be ran. In this case this is not Rust. (there's also no_compile)

Kixunil
Kixunil approved these changes Dec 1, 2022
View reviewed changes
Copy link
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 13d94cb

@tcharding
Copy link
Member Author
tcharding commented Dec 1, 2022 •
edited
Loading

Note that no_run isn't just convention. When specified alone (or I think with rust) the code will be compiled as Rust to check that it compiles but won't be ran. In this case this is not Rust. (there's also no_compile)

EDIT: Updated to the correct rules for later readers

I tried a bunch of combinations and came up with the surprising rule set:

  • ``` and ```rust : builds and runs
  • ```rust and ```rust,no_run : builds and does not run
  • ```<any other characters> : does not build and [obviously] does not run

@Kixunil
Copy link
Collaborator
Kixunil commented Dec 1, 2022

Also the difference between no_compile and some_other_language is that the former uses Rust highlighting, the latter doesn't

@tcharding
Copy link
Member Author

I did not check HTML for all the combinations :)

@Kixunil
Copy link
Collaborator
Kixunil commented Dec 1, 2022

Oh, rust,no_run works and no_run doesn't. That's confusing! But the documentation says something else. So no_run isn't a convention but a bug. :)
All blocks should have the correct language or text for plain text.

@Kixunil
Copy link
Collaborator
Kixunil commented Dec 1, 2022

no_run : does not get built

You seem to be doing something wrong:

/// ```no_run
/// syntax error
/// assert_eq!(2 + 2, 5);
/// ```
pub struct Foo;

cargo test fails compilation.

@Kixunil
Copy link
Collaborator
Kixunil commented Dec 1, 2022

Also, I misremembered, there's no no_compile but ignore does what I meant.

@tcharding
Copy link
Member Author

Oh damn, I was "breaking" the code with // something-obvisouly-not-code but it does not run because of only using two slashes :( With three slashes it does as you say. Point to you :)

@tcharding
Copy link
Member Author

Will fix my rules list later, got to run now.

sanket1729
sanket1729 approved these changes Dec 2, 2022
View reviewed changes
Copy link
Member
@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK 13d94cb

@sr-gi
Copy link
Contributor
sr-gi commented Dec 2, 2022

ACK 13d94cb

@Kixunil Kixunil added documentation trivial Obvious, easy and quick to review (few lines or doc-only...) labels Dec 2, 2022
apoelstra
apoelstra approved these changes Dec 2, 2022
View reviewed changes
Copy link
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 13d94cb

@apoelstra apoelstra merged commit af1d22b into rust-bitcoin:master Dec 2, 2022
@tcharding tcharding deleted the 12-02-rm-no-run branch December 4, 2022 22:17
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

@sanket1729 sanket1729 sanket1729 approved these changes

Assignees
No one assigned
Labels
trivial Obvious, easy and quick to review (few lines or doc-only...)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tcharding @Kixunil @sr-gi @apoelstra @sanket1729
0