House-keeping Improvements#792
Conversation
| contents: read | ||
|
|
||
| env: | ||
| WARNINGS_AS_ERRORS: yes |
There was a problem hiding this comment.
This was apparently useless?
Maybe we had some custom logic using it in the past, but nothing in Rust checks or cares about this environment variable.
| env: | ||
| RUSTDOCFLAGS: "-D warnings" |
There was a problem hiding this comment.
Better to set this for all jobs, instead of doing it for every single one.
| with: | ||
| files: | | ||
| **/*.{rs,md,json} | ||
| **/*.{rs,slice,md,json,yml,toml} |
There was a problem hiding this comment.
Spell check everything!
| branches: ['main'] | ||
| pull_request: | ||
| branches: ['main'] | ||
|
|
There was a problem hiding this comment.
For parity with our other workflow files.
There was a problem hiding this comment.
I went through and checked which of these words was actually still necessary, since we've pruned and removed alot of code from slicec after dropping Slice1 support.
Sure enough, many of them are no longer necessary.
| # Remove Cargo.lock from gitignore if creating an executable, leave it for libraries | ||
| # More information here https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html | ||
| Cargo.lock |
There was a problem hiding this comment.
The Rust team has softened their stance about including the Cargo.lock file.
Including it gives us more reproducible builds but does some harm to the ecosystem since we won't be testing against the latest and greatest anymore.
By making sure to update our dependencies more often, this becomes a non-issue though.
| @@ -76,7 +76,7 @@ mod fixed_size { | |||
| ); | |||
| let remaining = &buffer[expected.len()..]; | |||
| assert!( | |||
There was a problem hiding this comment.
Also fixed some random issues I found by running the latest version of the linter.
All very minor things.
There was a problem hiding this comment.
Pull request overview
Housekeeping-focused cleanup across tests, workspace metadata, and CI/spellcheck workflows to reduce duplication and improve consistency.
Changes:
- Simplifies a few Rust tests (idiomatic assertions, avoids unnecessary
format!, minor iterator cleanup). - Moves crate
repositorymetadata into the workspace package section and adds a workspace-levelCargo.lock. - Updates CI to enforce warnings-as-errors via
RUSTFLAGS/RUSTDOCFLAGSand expands spellcheck to more file types.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| slicec/tests/enums/container.rs | Uses is_empty() for clearer empty-field assertion. |
| slicec/tests/custom_tests.rs | Replaces format! with a static &str Slice snippet. |
| slicec/tests/attribute_tests.rs | Uses a fixed array instead of vec! for expected lints. |
| slicec/Cargo.toml | Inherits repository from workspace package metadata. |
| slice-codec/tests/encoding_tests.rs | Minor idiomatic iterator and UTF-8 byte count simplifications. |
| slice-codec/Cargo.toml | Inherits repository from workspace package metadata. |
| Cargo.toml | Adds workspace-level repository value. |
| Cargo.lock | Adds committed lockfile for the workspace. |
| .vscode/cspell.json | Updates allowed-words list (but see review comment). |
| .gitignore | Stops ignoring Cargo.lock. |
| .github/workflows/spellcheck.yml | Runs spellcheck on main pushes/PRs and expands checked extensions. |
| .github/workflows/ci.yml | Enforces warnings as errors via flags and removes per-step doc flag override. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
While giving a fresh slate to https://github.com/icerpc/icerpc-rust/pull/26, I decided to look back into some of the things we were setting/doing in these ancillary files, and sure enough there was some cleanup/improvement to do.
They're all minor enough that I think not backporting anything is perfectly fine.