Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Add an example to read an environment variable #422

wants to merge 1 commit into from

Conversation

Dineshs91
Copy link

fixes #401

@AndyGauge
Copy link
Collaborator

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");
Copy link
Collaborator

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

Copy link
Collaborator

@AndyGauge AndyGauge left a 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.

@AndyGauge
Copy link
Collaborator

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.

@Dineshs91
Copy link
Author

@AndyGauge Thanks for the quick response. I will update the code to work on windows as well.

@Dineshs91
Copy link
Author

I've updated the code and removed all the file operations. I will squash the commits once everything is fine with this PR.

@AndyGauge
Copy link
Collaborator

I forgot to mention putting in a few lines about the code, but you took care of that already! Thanks.

@Dineshs91
Copy link
Author

@AndyGauge I've squashed the commits.

Copy link
Collaborator

@budziq budziq left a 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! {
Copy link
Collaborator

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
Copy link
Collaborator

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() {
Copy link
Collaborator

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);
Copy link
Collaborator

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") {
Copy link
Collaborator

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();
Copy link
Collaborator

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?

@AndyGauge
Copy link
Collaborator

@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.

@AndyGauge AndyGauge closed this Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open Config file from given environment variable location
3 participants