Skip to content

[Merged by Bors] - Update linters.md with info about cargo ci xtask#1507

Closed
CleanCut wants to merge 4 commits intobevyengine:mainfrom
CleanCut:patch-1
Closed

[Merged by Bors] - Update linters.md with info about cargo ci xtask#1507
CleanCut wants to merge 4 commits intobevyengine:mainfrom
CleanCut:patch-1

Conversation

@CleanCut
Copy link
Member

Update linters.md with info about cargo ci xtask as per #1463 (review)

Update `linters.md` with info about `cargo ci` xtask as per #1463 (review)
@alice-i-cecile alice-i-cecile added A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Feb 23, 2021
@CleanCut
Copy link
Member Author

@alice-i-cecile You're quick! By the time I thought to go add the labels, you'd already done it. 😄

@CleanCut CleanCut added the C-Docs An addition or correction to our documentation label Feb 23, 2021
@DJMcNab
Copy link
Member

DJMcNab commented Feb 23, 2021

I don't actually think that command works

That is, we don't actually have a .cargo/config.toml to add the command.

@MinerSebas
Copy link
Contributor

MinerSebas commented Feb 23, 2021

I would rewrite the File like this:

## [xtask](https://github.com/matklad/cargo-xtask)

For simple cross-platform rust linting run the following command:

```bash
cargo run --package ci
```

### [rustfmt](https://github.com/rust-lang/rustfmt)

Is automatically run through [xtask](#xtask) (which also runs other checks) or manually with this command:

```bash
cargo fmt --all
```

### [Clippy](https://github.com/rust-lang/rust-clippy)

Is automatically run through [xtask](#xtask) (which also runs other checks) or manually with this command:

```bash
cargo clippy --workspace --all-targets --all-features -- -D warnings -A clippy::type_complexity -A clippy::manual-strip
```

This PR should also wait until #1508 is merged or closed, as it switches away from xtask.

@CleanCut
Copy link
Member Author

CleanCut commented Feb 24, 2021

I don't actually think that command works

That is, we don't actually have a .cargo/config.toml to add the command.

@DJMcNab Thank you for finding that! No wonder the command alias wasn't working in CI! .cargo/config.toml is listed in .gitignore!!! I'm surprised I didn't see anyone else reporting that it was broken for them. I did add .cargo/config.toml locally and it's been working fine for me, but it never made it into my PRs. I hadn't seen that my "git add -A" didn't actually add it.

I added the file to #1508

@CleanCut
Copy link
Member Author

This PR should also wait until #1508 is merged or closed, as it switches away from xtask.

@MinerSebas I don't think this documentation update needs to be blocked on #1508. "xtask" is a pattern of using a cargo command alias and a Rust project to do something that would have been traditionally done in bash. #1508 preserves that pattern of using a Rust project and cargo alias (in fact, #1508 fixes the broken alias part), it just uses a different library for executing external commands (duct instead of xshell) to make it easier to cut down on unwanted warnings.

My takeaway from your content suggestion is that the "xtask" terminology is confusing. That's probably true -- there's no need for the doc to specify the implementation pattern we're using to accomplish things. I've pushed up a new commit to try to simplify the wording further. What do you think of the updated wording?

@DJMcNab
Copy link
Member

DJMcNab commented Feb 24, 2021

Personally, I'd prefer if we used the literal term xtask for our tasks

Then we could have cargo xtask fmt cargo xtask ci, cargo xtask clippy for example.

@cart cart removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 14, 2021
@cart
Copy link
Member

cart commented Apr 12, 2021

bors r+

bors bot pushed a commit that referenced this pull request Apr 12, 2021
Update `linters.md` with info about `cargo ci` xtask as per #1463 (review)

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors
Copy link
Contributor

bors bot commented Apr 12, 2021

@bors bors bot changed the title Update linters.md with info about cargo ci xtask [Merged by Bors] - Update linters.md with info about cargo ci xtask Apr 12, 2021
@bors bors bot closed this Apr 12, 2021
jihiggins pushed a commit to jihiggins/bevy that referenced this pull request Apr 18, 2021
Update `linters.md` with info about `cargo ci` xtask as per bevyengine#1463 (review)

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
Update `linters.md` with info about `cargo ci` xtask as per bevyengine#1463 (review)

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change C-Docs An addition or correction to our documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants