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

Update contributing guide with more details on Rust process #13170

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mtreinish
Copy link
Member

Summary

As we're using an increasing amount of rust in Qiskit (as of this commit the repository is roughly 17% Rust by lines of code) the CONTRIBUTING.md file was a bit out of date as the guidance on working with Rust was a bit lacking. As an increasing number of code contributors to Qiskit will likely be writing some Rust code updating the guidance to be a bit more clear will be helpful. This commit goes through and expands on some details of working with and contributing Rust code. It also updates some other small details in the guide which were out of date.

Details and comments

As we're using an increasing amount of rust in Qiskit (as of this commit
the repository is roughly 17% Rust by lines of code) the CONTRIBUTING.md
file was a bit out of date as the guidance on working with Rust was a
bit lacking. As an increasing number of code contributors to Qiskit will
likely be writing some Rust code updating the guidance to be a bit more
clear will be helpful. This commit goes through and expands on some
details of working with and contributing Rust code. It also updates some
other small details in the guide which were out of date.
@mtreinish mtreinish added documentation Something is not clear or an error documentation Changelog: None Do not include in changelog labels Sep 17, 2024
@mtreinish mtreinish requested a review from a team as a code owner September 17, 2024 18:49
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@coveralls
Copy link

Pull Request Test Coverage Report for Build 10909185640

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 14 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.004%) to 88.91%

Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 2 88.26%
crates/qasm2/src/lex.rs 6 91.98%
qiskit/synthesis/two_qubit/xx_decompose/decomposer.py 6 90.91%
Totals Coverage Status
Change from base Build 10892476121: 0.004%
Covered Lines: 73467
Relevant Lines: 82631

💛 - Coveralls

Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

Looks good, just one small typo / extra word.

code formatting check) you can use `tox -eblack` to automatically
fix update the code formatting.
or rust code formatting check) you can use `tox -eblack` and
`cargo fmt` to automatically fix update the code formatting.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`cargo fmt` to automatically fix update the code formatting.
`cargo fmt` to automatically fix the code formatting.

@@ -187,8 +185,8 @@ please ensure that:
which will run these checks and report any issues.

If your code fails the local style checks (specifically the black
code formatting check) you can use `tox -eblack` to automatically
fix update the code formatting.
or rust code formatting check) you can use `tox -eblack` and
Copy link
Member

Choose a reason for hiding this comment

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

I like how you corrected "rust" to "Rust" in other parts of the document, then introduced some more lower case here lol

Comment on lines +584 to +586
For more detailed guidance on how to add Rust testing you can refer to the Rust
documentation's [guide on writing tests](https://doc.rust-lang.org/book/ch11-01-writing-tests.html)
for more details and suggestions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
For more detailed guidance on how to add Rust testing you can refer to the Rust
documentation's [guide on writing tests](https://doc.rust-lang.org/book/ch11-01-writing-tests.html)
for more details and suggestions.
For more detailed guidance on how to add Rust testing you can refer to the Rust
documentation's [guide on writing tests](https://doc.rust-lang.org/book/ch11-01-writing-tests.html).

Comment on lines +655 to +665
If you're working on Rust code the Rust formatting and lint checking are done
using different tooling. Qiskit uses [rustfmt](https://github.com/rust-lang/rustfmt) for
code formatting, and you can simply run `cargo fmt` (if you installed Rust with the
default settings using `rustup`) and it will update the code formating automatically to
conform to the style guidelines. This is very similar manner to what `black` and running
`tox -eblack` for Python code. For lint Qiskit uses [clippy](https://github.com/rust-lang/rust-clippy)
which can be invoked via `cargo clippy`. For CI to pass you will need clippy to pass
without any warnings or errors.

These still get checked via the tox lint commands but if you need to update formating
you'll need to run ``cargo fmt``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If you're working on Rust code the Rust formatting and lint checking are done
using different tooling. Qiskit uses [rustfmt](https://github.com/rust-lang/rustfmt) for
code formatting, and you can simply run `cargo fmt` (if you installed Rust with the
default settings using `rustup`) and it will update the code formating automatically to
conform to the style guidelines. This is very similar manner to what `black` and running
`tox -eblack` for Python code. For lint Qiskit uses [clippy](https://github.com/rust-lang/rust-clippy)
which can be invoked via `cargo clippy`. For CI to pass you will need clippy to pass
without any warnings or errors.
These still get checked via the tox lint commands but if you need to update formating
you'll need to run ``cargo fmt``.
For formatting and lint checking Rust code, you'll need to use different tools than you would for Python. Qiskit uses [rustfmt](https://github.com/rust-lang/rustfmt) for
code formatting. You can simply run `cargo fmt` (if you installed Rust with the
default settings using `rustup`), and it will update the code formatting automatically to
conform to the style guidelines. This is very similar to running `tox -eblack` for Python code. For lint checking, Qiskit uses [clippy](https://github.com/rust-lang/rust-clippy) which can be invoked via `cargo clippy`.
Rust lint and formatting checks are included in the the `tox -elint` command. For CI to pass you will need both checks to pass without any warnings or errors. Note that this command checks the code but won't apply any modifications, if you need to update formatting, you'll need to run `cargo fmt`.

Comment on lines +568 to +572
Many Rust-accelerated functions are generally tested from Python space, but in cases
where new Rust-native APIs are being added or there is Rust-specific internal details
to be tested, `#[test]` functions can be included inline. Typically it's most
convenient to place these in a separate inline module that is only conditionally
compiled in, such as
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Many Rust-accelerated functions are generally tested from Python space, but in cases
where new Rust-native APIs are being added or there is Rust-specific internal details
to be tested, `#[test]` functions can be included inline. Typically it's most
convenient to place these in a separate inline module that is only conditionally
compiled in, such as
Many Rust-accelerated functions are generally tested from Python space, but in cases
where new Rust-native APIs are being added, or there are Rust-specific internal details
to be tested, `#[test]` functions can be included inline. It's typically most
convenient to place these in a separate inline module that is only conditionally
compiled in, such as

@@ -187,8 +185,8 @@ please ensure that:
which will run these checks and report any issues.

If your code fails the local style checks (specifically the black
code formatting check) you can use `tox -eblack` to automatically
fix update the code formatting.
or rust code formatting check) you can use `tox -eblack` and
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
or rust code formatting check) you can use `tox -eblack` and
or Rust code formatting check) you can use `tox -eblack` and

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog documentation Something is not clear or an error documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants