-
Notifications
You must be signed in to change notification settings - Fork 62
Reduce duplication, avoid warnings #102
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
Usage was removed in commit ddb56a8 ("fixes").
This check became obsolete in commit a8a30d5 ("further working towards parsing github spec") with the removal of the `self.content.len() != 1` check.
This avoids a compilation warning: > warning: virtual workspace defaulting to `resolver = "1"` despite one or more workspace members being on edition 2021 which implies `resolver = "2"` > note: to keep the current resolver, specify `workspace.resolver = "1"` in the workspace root's manifest > note: to use the edition 2021 resolver, specify `workspace.resolver = "2"` in the workspace root's manifest > note: for more details see https://doc.rust-lang.org/cargo/reference/resolver.html#resolver-versions > Blocking waiting for file lock on build directory
|
Thanks for putting this together. I'll have a look at this week. |
|
@augustuswm gentle ping. |
|
Apologies here! Time got away from me. I'm traveling and won't get to this until next week. But please do ping me again if I don't. |
|
@augustuswm ping. |
| let (body_param, body_func) = if let Some(b) = &o.request_body { | ||
| if let Ok(b) = b.item() { | ||
| if b.is_binary()? { | ||
| if crate::is_binary(b)? { |
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.
What is the measurable value to moving this to a free function?
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.
It's explained in the commit message, and it's very clear if you look at the commit by itself: a78ba1a?w=1
it removes one copy of a function.
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 understand the change, I was more asking if this has a measurable impact on compilation or runtime performance? I was not able to see it initially on my machine, but that is only one data point.
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 didn't perform science. I saw duplicated code and removed it.
| updates: | ||
| - package-ecosystem: "cargo" # See documentation for possible values | ||
| directory: "/generator" # Location of package manifests | ||
| - package-ecosystem: "cargo" |
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 would prefer to move these changes to a separate PR to consider them independently.
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.
They're in a separate commit. I can send a separate PR.
|
@augustuswm ping, another 10 days gone by. FWIW I no longer use this library (its incredible long build times finally broke me), so I'm happy to close this if you'd rather. |
@jessfraz could you have a look please? Best reviewed commit-by-commit.