5 ms·

> new contributors make PRs. they make silly simple mistakes due to lack of experience; you point them out and they get fixed. this can be fun, for a time. what

> new contributors make PRs. they make silly simple mistakes due to lack of experience; you point them out and they get fixed. this can be fun, for a time. what it’s teaching you is that you personally are responsible for catching mistakes. Every team I've worked with that was feeling exhausted about code contributions actually needed to focus on better documenting guidelines but most importantly, improve CI. Catching silly mistakes is the textbook definition where automating stuff can help the most. This should improve reviewer's peace of mind and speed things up because people will work by themselves to fix things before asking for a review. I don't know what's the CI situation in the Rust project but if it's anything like what I'm used to, it probably needs improvement. Adding more human-hours to reviewing things isn't sustainable.

raverbashing 3y ago
I agree At the same time lint tools sometimes move into the 'pedantic unhelpful' field because those things are the easiest to check (no, nobody likes pep8 and its brain-dead char limit - Black is much better) But yes, if it can be automated it should be automated
jynelson 3y ago
rust's CI is quite extensive actually; there's a talk on it here: https://www.pietroalbini.org/blog/shipping-a-compiler-every-six-weeks/ https://www.pietroalbini.org/blog/shipping-a-compiler-every-... CI is good. more tests are good. but there's a limit to how much they can catch. here is an example: there is an internal type checking API called [`resolve_vars_if_possible`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/infer/struct.InferCtxt.html#method.resolve_vars_if_possible https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/...). this needs to be called on the output of almost any call to `.normalize()`, or you will get an internal compiler error. the conditions under which you get an ICE are quite complicated; sometimes you need 6 or more items in the crate with `impl Trait` or type_alias_impl_trait or const generics involved. but it's very easy to see there's a bug just by looking at the code and seeing the missing `resolve_vars()` call. how can we automate this? well, rustc has a mechanism for "internal lints", but they're somewhat annoying to write - you're basically writing a new compiler pass, and it needs to not have false positives. it's much easier to just catch this one instance. so the reviewer, who's already burned out, does this quick manual review instead.
- symlinkk 3y ago
  Just tell that verbatim to ChatGPT and it will catch it for you.
- pas 3y ago
  hm, is it possible to ... add these trivial-by-look-but-hard-to-script things to a checklist as an auto-comment to PRs? is there a list of these? anyway, thanks for writing this post, and for featuring that awesome message/art/logo/thing at the end \m/
  - jynelson 3y ago
    yes, it's possible! that list doesn't exist today but i would love to create it. i wrote a draft a few years ago before shifting to other work; someone recently expressed interest in reviving that project: https://github.com/rust-lang/rustc-dev-guide/pull/1463 https://github.com/rust-lang/rustc-dev-guide/pull/1463 <3 i'm glad you enjoyed it
chubot 3y ago
This is true, but then the corrollary is that new PRs need to come with this higher, rigorous level of test coverage. And then that becomes a bit of a barrier to contribution -- it's a tradeoff I often write an entirely new test harnesses for a feature, e.g. for https://www.oilshell.org https://www.oilshell.org, particularly using other implementations as "oracles". All of these run in the CI - https://www.oilshell.org/release/latest/quality.html https://www.oilshell.org/release/latest/quality.html The good thing is that it definitely helps me accept PRs faster. Current contributors are good at this kind of exhaustive testing, but many PRs aren't Even so, I do find myself simply adding test coverage from time-to-time -- that's an important job of a maintainer

raverbashing 3y ago

I agree At the same time lint tools sometimes move into the 'pedantic unhelpful' field because those things are the easiest to check (no, nobody likes pep8 and its brain-dead char limit - Black is much better) But yes, if it can be automated it should be automated

jynelson 3y ago

rust's CI is quite extensive actually; there's a talk on it here: https://www.pietroalbini.org/blog/shipping-a-compiler-every-six-weeks/ https://www.pietroalbini.org/blog/shipping-a-compiler-every-... CI is good. more tests are good. but there's a limit to how much they can catch. here is an example: there is an internal type checking API called [`resolve_vars_if_possible`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/infer/struct.InferCtxt.html#method.resolve_vars_if_possible https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/...). this needs to be called on the output of almost any call to `.normalize()`, or you will get an internal compiler error. the conditions under which you get an ICE are quite complicated; sometimes you need 6 or more items in the crate with `impl Trait` or type_alias_impl_trait or const generics involved. but it's very easy to see there's a bug just by looking at the code and seeing the missing `resolve_vars()` call. how can we automate this? well, rustc has a mechanism for "internal lints", but they're somewhat annoying to write - you're basically writing a new compiler pass, and it needs to not have false positives. it's much easier to just catch this one instance. so the reviewer, who's already burned out, does this quick manual review instead.

symlinkk 3y ago
Just tell that verbatim to ChatGPT and it will catch it for you.
pas 3y ago
hm, is it possible to ... add these trivial-by-look-but-hard-to-script things to a checklist as an auto-comment to PRs? is there a list of these? anyway, thanks for writing this post, and for featuring that awesome message/art/logo/thing at the end \m/
- jynelson 3y ago
  yes, it's possible! that list doesn't exist today but i would love to create it. i wrote a draft a few years ago before shifting to other work; someone recently expressed interest in reviving that project: https://github.com/rust-lang/rustc-dev-guide/pull/1463 https://github.com/rust-lang/rustc-dev-guide/pull/1463 <3 i'm glad you enjoyed it

chubot 3y ago

This is true, but then the corrollary is that new PRs need to come with this higher, rigorous level of test coverage. And then that becomes a bit of a barrier to contribution -- it's a tradeoff I often write an entirely new test harnesses for a feature, e.g. for https://www.oilshell.org https://www.oilshell.org, particularly using other implementations as "oracles". All of these run in the CI - https://www.oilshell.org/release/latest/quality.html https://www.oilshell.org/release/latest/quality.html The good thing is that it definitely helps me accept PRs faster. Current contributors are good at this kind of exhaustive testing, but many PRs aren't Even so, I do find myself simply adding test coverage from time-to-time -- that's an important job of a maintainer