-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Marking Cargo.lock as generated #6548
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
Thanks! I wonder if we could perhaps expand the comment a bit? Something along the lines of adding human-readable text about how it's a generated file and should not be edited, followed by |
c437a8f
to
79d8166
Compare
Sure! I was a little unsure, so I pushed my attempt. WDYT? I'm happy to keep tweaking it. |
I used https://github.com/github/linguist/blob/f1f4018d7a9950c1d9cf3d0887d994cf496d63c4/lib/linguist/generated.rb as inspiration (note Cargo.lock is already ignored there). Specifically, I preferred "Not intended for manual editing" over "Do not edit" (or "DO NOT EDIT"). |
This is going to affect every single Rust project and likely surprise a lot of people. I think we're ready for this change though and the surprise is mostly going to be a result of change, not that it's a change that shouldn't have happened. Regardless I'd like to make sure we're all on board with this, so.. @rfcbot fcp merge |
Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
79d8166
to
1f4b085
Compare
Just to be clear, the only effect is that if you check in the Cargo.lock into VCS, it will show up as dirty after the first build with a new cargo? If you don't check the lock file in, you probably won't even notice? |
@ehuss an excellent clarification, and spot on |
src/cargo/ops/lockfile.rs
Outdated
// https://github.com/rust-lang/cargo/issues/6180 | ||
// At the start of the file we notify the reader that the file is generated. | ||
// Specifically Phabricator ignores files containing "@generated", so we use that. | ||
let marker_line = "# This file is automatically @generated. Not intended for manual editing."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excuse me, why there is two spaces here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔔 This is now entering its final comment period, as per the review above. 🔔 psst @alexcrichton, I wasn't able to add the |
Maybe nitpicking but could we have a full English sentence instead of "Not intended for manual editing" here? E.g. "This file is not intended for manual editing" or "It is not ...". Also, if this is an auto-generated egg, where is the chicken - who generated it?
|
@dwijnand want to follow up with @lukaslueg's thoughts and we can merge? |
1f4b085
to
113ae77
Compare
I thought it was fine, but I don't mind tweaking it. I dropped the "automatically" because I want to keep it one line, under 80 chars, as looking in https://github.com/github/linguist/blob/f1f4018d7a9950c1d9cf3d0887d994cf496d63c4/lib/linguist/generated.rb it's probably best, over multi-line markers. |
Hm I'm not sure I understand the restriction of one line, is that something that tools conventionally try to accept? |
113ae77
to
bd0e4a0
Compare
Yes, it would appear so, from that linguist file. But I realised we can just call the first one the marker line. So it's now two lines: # This file is automatically @generated by Cargo.
# It is not intended for manual editing. |
@bors: r+ |
📌 Commit bd0e4a0 has been approved by |
…hton Marking Cargo.lock as generated 12 weeks have passed, let's mark lock files as generated! Fixes #6180
☀️ Test successful - checks-travis, status-appveyor |
12 weeks have passed, let's mark lock files as generated!
Fixes #6180