chore: Use inheritance to reduce copy-paste in Cargo.tomls
#7687
Conversation
akhi3030
force-pushed
the
akhi3030/cargo-inheritance
branch
from
3f8aea0
to
636148d
Compare
| } | ||
| None => { | ||
| // we can skip, since we have has_rust_version check | ||
| continue; |
There was a problem hiding this comment.
Re-reading this bit of code, I think we can just remove the whole function? We check that rust-version is specified at all in has_rust_version, and it cant' actually be greated that what we have in rust-toolcahin, as that would break the build.
I suggest sending a follow-up just removing this logic (but maybe I am missing something obvious here?)
There was a problem hiding this comment.
For future PRs which combine "do the same trivail change all over the place" with "do small non-trivial change in one place", consider more explicitly calling this structure out, so that the interesting bit isn't missed. (eg, by adding a comment yourself on GitHub, or setting-up history for per-commit review.)
There was a problem hiding this comment.
Re-reading this bit of code, I think we can just remove the whole function? We check that rust-version is specified at all in has_rust_version, and it cant' actually be greated that what we have in rust-toolcahin, as that would break the build.
Hmm... that might indeed be the case. I'll investigate a bit. I was actually thinking of doing something simpler and combining has_rust_version and has_debuggable_rust_version. They seem to be performing similar checks.
For future PRs which combine "do the same trivail change all over the place" with "do small non-trivial change in one place", consider more explicitly calling this structure out, so that the interesting bit isn't missed. (eg, by adding a comment yourself on GitHub, or setting-up history for per-commit review.)
Thanks for the feedback. I will keep it in mind in future.
See #7674 for more details.