chore: Tweaked .gitignore #397
Conversation
- Made .gitignore alphabetical. - Adjusted ignore paths.
WalkthroughThe changes involve updates to the Changes
Poem
Note Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://coderabbit.ai TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Here's some arguments in favor of committing these - https://doc.rust-lang.org/cargo/faq.html#why-have-cargolock-in-version-control |
|
@miraclx don't get me wrong - there are definitely some reasons to commit lockfiles! 🙂 However, generally speaking, committing build artifacts is discouraged, and what I've observed is that there are lots of changes to the lockfiles along with code changes, without having necessity or thoughtful context. What I mean by that is, at present when a change is made, if the lockfile changes also, then it is simply committed. This therefore does not have a lot of value and causes noise in the commit history. If we omit it from Git, there is no particular side effect, save of course a situation of wanting to know exactly what version of a crate was used for a build. Typically that can be found from the build pipeline if actually needed. So I think if there is a conscious reason to commit the lockfiles, by all means we can, but it's what I'd consider non-standard and might need some thought as to how and when we update them? |
|
...just as a further thought - the nature of the crate does come into it as well 🤔 I think it's more acceptable to commit a lockfile for an application, but it's definitely frowned on/discouraged to include them for libraries. |
|
Hmmmm, I have always been used to I don't remember reading or hearing anything about this, so I guess it's been somewhat silent and stayed under the radar. Perhaps my perspective/thinking is out-of-date? I still think excluding lockfiles is the ideal approach, but I need to carefully consider the reasons in that Cargo change. It might be that we can work out an approach whereby lockfiles are not updated in code change branches unnecessarily? Would you like me to amend this PR to remove the lockfile removal while this is investigated/discussed further? |
.gitignore
Outdated
| @@ -1,13 +1,16 @@ | |||
| .idea/ | |||
| res/ | |||
| Cargo.lock | |||
There was a problem hiding this comment.
Why are we not committing Cargo.lock? Am I missing something big?
There was a problem hiding this comment.
@fbozic see discussion above 🙂 I hope that helps?
There was a problem hiding this comment.
yeah, sorry missed it cos I've opened it from email directly on the review page
.gitignore
Outdated
| *.map | ||
| pnpm-lock.yaml |
There was a problem hiding this comment.
Why are we not committing pnmp-lock.yaml? Am I missing something big?
There was a problem hiding this comment.
@fbozic see discussion above 🙂 I hope that helps?
There was a problem hiding this comment.
yeah, sorry missed it cos I've opened it from email directly on the review page
|
Sorry, missed the comments outside the review. I think we are going to break CI pipeline for the JS sdk because we are using frozen deps (or at least we should). |
|
@fbozic no worries! 😄 That makes sense. I will amend this PR accordingly. I wonder, though, if we can work out a process by which we avoid undue lockfile noise in the commit history? 🤔 |
Let's do it 🚢 |
danwilliams
force-pushed
the
chore/fix-gitignore
branch
from
9ea3b3f to
f7de567
Compare
I am not sure if this ever happened to me. Do you maybe have an example of change where lock file shouldn't be changed but it was? Yesterday I was in a cleane up mode and I did few PRs and all the changes in the lock file looked reasonable to me; PR with lock changes which are expected because I've removed last usage of libp2p with all features, PR without lock changes because there is another crate which uses libp2p with all features. I would like to address the root cause of the issue rather than removing lock files. |
|
@xilosada Done 🙂 The change hadn't broken any of the checks made here, but I can understand if it's needed for that kind of purpose, perhaps elsewhere. It does mean there's not much left in this change lol 😅 |
The pattern I have always been used to for many years is to exclude all build artifacts from repositories as a matter of best practice. There are occasions where they may be needed, but generally speaking, as a baseline, avoiding their inclusion is generally considered desirable. In this situation it seems we have a valid reason to include them, which is fine 🙂 I wonder if it might be worth making a note about this somewhere, along with guidance for when they should be updated? I for instance try to avoid updating them in a code-change branch unless specifically making a change to |
I'm all for removing build artifacts from repositories, I'm just not sure which artifacts are you referring to. I can see that |
|
@fbozic absolutely, there is a nuance between artifacts like that, and the lockfiles 🙂 Bear in mind, I have seen valid cases for all build artifacts being added, for reasons along the same lines (reproducibility) - despite how yucky that feels! 😅 With lockfiles, I generally see those as an output of the build process because they are a generated file (not manually created or edited) and will align to the directives in It is of course possible to lock down the versions in use by specifying exact versions rather than compatible ones, but that tends to be annoying. I think the common thinking has for some time been that if something is compatible, it should be fine to build, and therefore the lockfile isn't needed in the (code) repo (although this is not guaranteed - such as the Chrono guys breaking things a couple of times earlier this year lol 😅). For projects using IaC, I've seen the lockfiles committed there along with the various recipes. That makes a lot of sense to me - code and the general associated requirements in the code repo, and build instructions along with lockfiles in an IaC repo. But there are many ways to do things, and this is just one of the ones I have observed. Given that Cargo has quietly changed its defaults, I do need to review the reasons behind that and perhaps update my opinion if it's now outdated! |
| target/ | ||
| target |
Tweaked
.gitignorefileRemoved
Cargo.lockandpnpm-lock.yamlfrom the.gitignorefile, as these should not be committed.Made
.gitignorealphabetical.Summary by CodeRabbit
.gitignoreto include/exclude specific directories and files.