Skip to content

Conversation

@BenjaminBrienen
Copy link
Contributor

  • Update workspace to all rust 2024
  • Update workspace to all rust-version 1.85
  • Fix cargo test --doc (mostly ignore things that aren't valid code)
  • cargo fmt

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 26, 2025
@ChayimFriedman2
Copy link
Contributor

We usually wait a bit before we transition to a new version, as some users are building rust-analyzer with non-bleeding-edge Rust.

@lnicola
Copy link
Member

lnicola commented Feb 26, 2025

We can check, but I have a feeling we no longer build on 1.84.

@BenjaminBrienen
Copy link
Contributor Author

Ok, no worries. It can just sit open until you decide to take action. I can fix merge conflicts if it says open for a while :)

@ChayimFriedman2
Copy link
Contributor

Some notes:

  1. cargo test already runs doctests, no need to run them separately (and also this is better done as a separate PR, this one is large enough).
  2. "2024" isn't a very descriptive commit message.
  3. Put the formatting changes in a commit for themselves, they're huge and not really meaningful.
  4. The same for cargo fix --edition, then put manual changes as a commit on top.
  5. And another commit to change the edition and MSRV.
  6. Please avoid formatting all TOML files by the way.

@BenjaminBrienen
Copy link
Contributor Author

The doc tests were failing, so I'm surprised to hear that they're already being run

@BenjaminBrienen
Copy link
Contributor Author

I'll redo this

@BenjaminBrienen
Copy link
Contributor Author

I don't understand why you don't use squash and merge practice.

@ChayimFriedman2
Copy link
Contributor

Squashing is fine, but we still like meaningful commits. Reviewing huge mostly machine-generated commits with a bit of manual code is very hard.

@lnicola
Copy link
Member

lnicola commented Feb 26, 2025

@BenjaminBrienen we used to use the two borses, for permission management and to serialize the tests, but we switched to merge queues a while ago. These don't let you squash:

image

@BenjaminBrienen
Copy link
Contributor Author

I think bevy uses both merge queues and squashing, but I could be wrong.

@Veykril
Copy link
Member

Veykril commented Feb 26, 2025

Please wait with this until #18964 has landed

@Veykril
Copy link
Member

Veykril commented Feb 26, 2025

Merge queues allow you to do either merges ,rebases or squashing but its a repo setting. Why would you want to squash a PR by default though, then you lose the history. Its much better to have the author squash themselves if the history is deemed unnecessary / convoluted

@BenjaminBrienen
Copy link
Contributor Author

I have arguments in favor of squashing all PRs, but I don't want to address it here

@BenjaminBrienen
Copy link
Contributor Author

cargo test definitely doesn't catch all the same things that cargo test --doc does.

@ChayimFriedman2
Copy link
Contributor

https://doc.rust-lang.org/stable/cargo/commands/cargo-test.html#target-selection

When no target selection options are given, cargo test will build the following targets of the selected packages:

  • ...
  • doc tests for the lib target

...

--doc
Test only the library’s documentation. This cannot be mixed with other target options.

@BenjaminBrienen
Copy link
Contributor Author

Idk what to tell you. cargo test passes, but cargo test --doc finds a bunch of errors.

@ChayimFriedman2
Copy link
Contributor

Well that sounds like a bug in Cargo then.

@ChayimFriedman2
Copy link
Contributor

Oh it's because we have doctest = false in Cargo.toml, so Cargo ignores it by default. I'm not sure we want to enable it; we have a lot of code snippets that are used to demonstrate how they are analyzed, and few that serve as actual doctests.

@BenjaminBrienen BenjaminBrienen deleted the 2024 branch February 26, 2025 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants