Guide: don't recommend xtask-fmt-on-save #914
Conversation
This doesn't work properly with xsync, it's slow, and it's of minimal value. Don't recommend it.
|
How critical are these checks? Do we have another (automated) way of checking compliance with these rules? |
|
Like I mentioned in the office hours chat - it's explicitly designed not to be slow. If we want to downgrade this from "recommended" to just "here's a thing you can do if you want" (or, maybe just adding the caveat that "while this works great with the main repo, there are known bugs when running in the context of the closed source repo") while we chase down the bug, we can do that. ...but removing this entirely seems a bit of an overreaction. I use this flow every day without issue, and I derive a lot of value from not having to wait for CI to re-run just because I forgot to add a copyright header to some new file I added. Talking with a few other folks offline - it seems others do as well. As for your question eric - if you don't use the run-on-save flow, then the same checks are run via a regular |
jstarks
changed the title
|
Come off it, it's broken. We have found multiple bugs with the fmt machinery lately, the kinds of bugs that I don't want hitting unsuspecting new contributors. CI will protect them from checking in formatting issues without this buggy format-on-save behavior. AFAICT, none of the other core maintainers actually follow this recommendation. It was authored by you. So, you either need to prioritize fixing the issues with it, or I am removing it. I am not going to add caveats to an already lengthy getting started guide. (This is already a compromise--I don't think this recommendation is worth the extra time and effort for new contributors.) |
|
I'd vote for mentioning somewhere early on in the guide (like our getting started section maybe?) that our CI will block on this check, and that it can be run locally. But agreed on not recommending it as run-on-save. |
|
My perception might be skewed here - from what I know, there's only a single bug specifically related to IIRC, the various other recent The fact we're having this conversation ~2+ years after this recommendation was added in the first place should be a testament to how this flow has been quietly working just fine for years, providing a nice bit of value to folks that appreciate having fmt tooling running as part of their core dev loop. As the original author of this fix-on-save code, sure, I'll take point on repro'ing and fixing this edge-case someone ran into. All I'm saying is that - while I'm well aware that many other core maintainers don't use all the bells-and-whistles that the IDE and/or the in-repo tooling offers - everyone decides where along the {syntax coloring, light IDE features, rust-analyzer, git hooks, fmt-on-save} gradient they want to sit. I don't think its fair to argue that there's any one "best" way to use / not-use dev-tools in order to be productive, and documenting what's available allows folks to make their own judgement calls here. |
|
I have a repro locally. Looking into it now. |
|
I've sent out PRs to resolve the issue. Once those land, I'll close this PR. |
Fixing the underlying issue from #914 This PR does a few things: 1. The `xtask-path` logic has been fixed to always drop the file under `target/xtask-path`, regardless of whether a `--custom-root` is being used or not. 2. A new `--run-on-save` parameter has been added to `xtask`, which can be used to skip certain expensive checks when running `xtask` in response to a file save event. It is not currently being used by the open-source `xtask`, but plays an important role in the internal repo's `xtask fmt` implementation. 3. Updates the Guide to make it more clear why one may or may not want to opt into this functionality. **These changes are not particularly interesting in isolation, but make more sense when reviewed alongside the changes being made in the internal repo.** The fact we have this sort of coupling between the oss xtask and the closed-source xtask is unfortunate, and something we should try and move away from in the future...
|
Thanks, Daniel. |
This doesn't work properly with xsync, it's slow, and it's of minimal
value. Don't recommend it.