-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Update pre-commit dependencies #16465
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'm going to wait before merging this PR as this is an upstream crate and I'm not sure what's the process of updating it is.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'm going to merge this, I've opened a PR to update this upstream as well. Happy to make any changes in a follow-up as needed.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this change is fine, but in general the idea is to minimize the diff between our code and upstream as much as possible. That way, if we want to bring in patches from upstream, it's less of a hassle. I don't know how long this will last or if it's a viable long term strategy, but it's what has been guiding my changes to our vendored copy so far. e.g., There's a lot of other stuff I would want to change too and general code clean-up and what not, but specifically decided against it to keep the diff minimal.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, that makes sense, I'll keep that in mind. The upstream PR is merged, so this shouldn't result in any diff. |
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.
We could change
CFormatErrorTypeto useCFormatErrorKindand usekindfield instead but I've avoided that for now astypstill needs to be ignored because it's used in third-party dependencies.TYPshould be removed once the redirect is removed.