removed json dependency #497
Conversation
There was a problem hiding this comment.
and updated some other dependencies
It's good to keep dependencies up-to-date, but we use dependabot (e.g. #488), so we don't really need to manually update dependencies 🙂
I believe it is generally best to keep unrelated changes out of a commit/PR. One of the uses of commits is to find which change introduced a bug (I trust that this PR doesn't introduce any bugs, though 😄 ). There are several ways to find the commit that introduced a bug, like git bisect (this is a very cool tool that sadly flies under the radar of many git users, I highly recommend looking it up). But when unrelated changes are included in a commit, it becomes more difficult to find exactly what's wrong.
Let's say, for example, that this PR did introduce a bug, and it was caused by updating regex. If we did a git bisect and found commit eed3ccd, this is the diff that we would see.
- git2 = {version = "0.13.22", default-features = false}
+ git2 = {version = "0.13.23", default-features = false}
image = "0.23.14"
- json = "0.12.4"
- regex = "1.4.6"
+ regex = "1.5.4"Looking at that diff, it isn't immediately obvious which change introduced a bug. But if you split this into 3 commits, remove json, update regex, and update git2, this is the diff that we would see.
- regex = "1.4.6"
+ regex = "1.5.4"As a general rule that I use personally, if I have to use "and" to describe a commit (e.g. "removed json and updated other dependencies"), it might be worth separating into multiple commits. Don't take this rule too strictly, though.
tl;dr I would recommend just removing the json dependency, and leaving git2 and regex alone.
| let parsed: serde_json::Value = serde_json::from_str(contents)?; | ||
| match &parsed["dependencies"].as_object() { | ||
| Some(val) => Ok(val.len()), | ||
| None => Ok(0), | ||
| } |
There was a problem hiding this comment.
👍 Thanks for this! I agree that using the existing dependency is better than adding a new dependency
revert update dependencies
Sorry for taking up your time and thanks for the advice, I'll keep it in mind in the future. I reversed the dependency update and left only the removal of the |
Don't worry about it, I chose to write that long rant when I could've just written the tl;dr. Sorry for taking up your time by making you read all that 🤣 |
removed json dependencies and updated some other dependencies