-
Notifications
You must be signed in to change notification settings - Fork 77
Cargo.lock handling to be same across examples #486
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
Conversation
|
I made a second branch, last week, the diff to Disclaimer: I'm not claiming this to be the truth. I'm just trying to make the Cargo.lock policy least invasive for me, as a developer, and keep it useful for the repo. I first thought my approach is disruptive, but Copilot sees it more as a continuation of the stated Cargo guidelines. (granted, Copilot has a tendency to agree; you be the judge of it) In short, I would:
This move keeps one or two Let me know what you think. If you show 👍 for the approach, I'll make the PR represent it, instead. ps. I asked Copilot to summarize the approach as the |
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.
Lgtm
03aa0af to
bff0ac4
Compare
bff0ac4 to
e7d0b5f
Compare
|
The CI states no space on device. I don't think I can fix that in any way? Let me know when there's a chance it might pass. |
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.
I'm sorry, this PR just doesn't make sense to me anymore, and changed a lot since I approved it.
- Remove the contributing.md - I don't think it's really needed to explain so much about the lock policy. It's just common rust pratice
- Examples should have lock files, don't remove them
This is how I want the .gitignore to look like:
**/target
**/target_ci
host/Cargo.lock
host-macros/Cargo.lock
bt-hci-linux/Cargo.lock
examples/apps/Cargo.lock
examples/tests/Cargo.lock
.idea
| **/target | ||
| **/target_ci | ||
| **/target/ | ||
| /target_ci/ |
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 should still be **/target_ci I think?
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.
There's only one target_ci used, ever, whereas target can be in the subdirs. I like to be explicit with these patterns - narrow is better than general.
|
@lulf I didn't intend to mislead, or hijack my own PR. I considered what you meant by "lgtm" and decided it was in the context of the above comment - thus went ahead and replaced the original approach with that. This was a mistake. But since I still think that approach - no lock files needed for examples - is better, I will:
I don't advocate the original change to the repo, any more. The branch I made in my fork remains available, if someone wants to use it as a reference. Sorry for the disturbance!!! |
I'm... having second thoughts about this, but since I volunteered in #482 to make one, here goes.
We can continue discussion here - the PR is intended to fulfil the specs in that discussion.