-
Notifications
You must be signed in to change notification settings - Fork 212
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
Adds 'try_parsing' option for Environment #137
Conversation
tests/env.rs
Outdated
|
||
let values = environment.collect().unwrap(); | ||
|
||
assert_eq!( |
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.
Maybe a beginner question, as I did not do much dev in this crate, but what is added value of this PR if you still have to use into_int().ok()
, so parse it again to retrieve final value?
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.
Yeah, on the surface it looks like that for sure. Only reason I had to do this is that tests in the crate don't expose the int directly, at least as far as I understand it. Seems to be because the Value
type exposed doesn't allow access to it's ValueKind. It's possible I could make a test that does deserialization with struct using serde instead?
Not that it's any consolation, but I have used this patch for projects of my own with success.
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 think I need to dig deeper in the code to fully grasp the added value then ;) Hope I'll find time to do it, this crate seems really nice and it would be good to ressurect it.
Are you still interested in this? If yes, I created a maintenance fork (read here). |
I think @johnb8 would be interested. |
Yeah I'm still interested in getting this merged. @matthiasbeyer looks like you're a maintainer here now so your fork isn't needed anymore? |
Yep, that's right. I'd like to wait for #175 and then ask to rebase this, and go from there. |
Please rebase. |
4289856
to
4c5cc21
Compare
Rebased on latest master |
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.
Overall, this looks good. But, I would like to see more tests for this kind of functionality.
First of all: Could you please split the added test into three test cases? This would be the first step. Don't worry about code duplication - I consider a certain level of code duplication in tests as necessity!
Second: Could you add more tests where the parsing is configured differently or fails entirely? Some where parsing fails, some where types do not match, some where types match and parsing is disabled... and so on!
Either way: Thanks for contributing to this crate!
Added some more environment variable parsing tests. Let me know if there's any more you want to see, I'm happy to add them. Thanks for stepping in to maintain this crate! |
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.
Okay, so another review for you here. I hope you don't mind my concerns.
src/env.rs
Outdated
if let (true, Ok(parsed)) = ( | ||
value.to_lowercase() == "true" || value.to_lowercase() == "false", | ||
string_value.clone().into_bool(), | ||
) { |
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 if
condition is rather hard to parse IMO. Can you rewrite it, so that the first part of the condition is a variable that is used in the if
? This way, this could fit into one line, which would make it way easier to parse for humans.
src/env.rs
Outdated
Value::new(Some(&uri), ValueKind::String(value)), | ||
); | ||
let value = if self.try_parsing { | ||
let string_value = Value::new(Some(&uri), ValueKind::String(value.clone())); |
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 a lot of clone()
ing involved here. Can we somehow reduce it?
This line clones, and later we clone the string_value
variable again in each if-let
... so in the worst-case, there are 5 clone()
calls. I bet this can be improved!
@matthiasbeyer I've implemented both of your suggestions. I don't mind your concerns, definitely agree with the excessive cloning |
Overall I like these changes here and I am looking forward on merging them. Two things, though:
I guess the Maybe you could add a Also, if we think this PR one step further, we might give the user the option to add own parsing functions - consider they want to pass something like |
This can be particularly helpful for `MY_SERVER_PORT=4334 cargo run`
Rebased to latest master and added performance note to Yeah I don't think we can do type inference with the way the I'll look into list parsing and try and get something working before we merge this |
Finally got around to review this. Looks good IMO, will merge now. |
This can be particularly helpful for
MY_SERVER_PORT=4334 cargo run
I know that serde's hints should do this because of the
deserialize_*
functions, but there are cases where it usesdeserialize_any
(for me this happened when refactoring to a nested struct in an enum). Maybe there's a better serde pattern for that/this?