-
Notifications
You must be signed in to change notification settings - Fork 287
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
Add an example to read an environment variable #422
Conversation
Thanks for the contribution. The AppVeyor might be issues with the new Rust version on Windows. Let me dig into that for you to make sure master is working as the errors seem to be unrelated to the code change. |
src/basics.md
Outdated
} | ||
|
||
fn set_env_variable() { | ||
env::set_var("CONFIG_FILE", "/tmp/config.toml"); |
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 is not a cross platform solution. I'm looking into the way the cookbook wants to recommend implementing file system abstractions. If you have any ideas, I'm interested. Right now I'm reading https://github.com/rust-lang-nursery/cli-wg/issues/10
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 need to make this compile on Windows. The example is also a bit larger than required. We don't need to operate on the file or create the default file. I do like the fallback to a default value when the file open operation fails.
OK, I dug into it and there is a symptom of new regression due to rust version update, but that is not causing the error in AppVeyor. |
@AndyGauge Thanks for the quick response. I will update the code to work on windows as well. |
I've updated the code and removed all the file operations. I will squash the commits once everything is fine with this PR. |
I forgot to mention putting in a few lines about the code, but you took care of that already! Thanks. |
@AndyGauge I've squashed the commits. |
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.
Hi @Dineshs91 thanks for contributing! Could you look into some of the comments below?
use std::env; | ||
use std::path::PathBuf; | ||
|
||
# error_chain! { |
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 example can be completely rewritten without error_chain
crate just change fn run() -> Result<()>
to fn main()
and remove Ok(())
and quick_main
with all error chain specific code
## Read environment variable | ||
[![std-badge]][std] [![cat-filesystem-badge]][cat-filesystem] | ||
|
||
Set an environment variable and read it back. If the variable is not found, then fallback |
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'd suggest to link to std documentation for std::env::set_var
and std::env::var
# } | ||
# } | ||
|
||
fn set_env_variable() { |
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'm not really sold on using separate function to wrap one liner here.
Also the duplication of the "config.toml" string in two places might be slightly confusing how about we use different values?
|
||
# error_chain! { | ||
# foreign_links { | ||
# Utf8(std::str::Utf8Error); |
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.
Neither of these are needed for this example
|
||
let default_config_file = "config.toml"; | ||
|
||
let config_file_path = match env::var("CONFIG_FILE") { |
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 might a matter of taste but how about rewriting this simple transformation from match to Result
combinators with something like:
let config_file_path = env::var("CONFIG_FILE")
.map(PathBuf::from)
.unwrap_or_else(|_| default_config_file.into());
} | ||
|
||
fn run() -> Result<()> { | ||
set_env_variable(); |
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.
setting and reading the same env variable does not make a control flow that is close to being realistic. How about we set one variable and read another?
@Dineshs91 would be happy to help you get the changes made to complete this. Please let me know if you are in need of support. |
fixes #401