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

feat: dotenv support #2587

Merged
merged 1 commit into from
Aug 3, 2022
Merged

feat: dotenv support #2587

merged 1 commit into from
Aug 3, 2022

Conversation

onbjerg
Copy link
Member

@onbjerg onbjerg commented Aug 3, 2022

Motivation

People want .env support

Solution

Loads .env before anything else so anything is configurable (NO_COLOR etc). Uses the dotenv crate which hasn't had an update in a while, but is widely used. Not sure if there are good alternatives.

Closes #840

@onbjerg onbjerg added the T-feature Type: feature label Aug 3, 2022
cli/src/term.rs Outdated Show resolved Hide resolved
Comment on lines +193 to +194
/// We could use `tracing::warn!` here, but that would imply that the dotenv file can't configure
/// the logging behavior of Foundry.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... because we would need to register the subscriber before loading .env

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will now auto load regardless which is a big change.

I think we should consider making this opt-in via Config?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure... if config is loaded, then dotenv won't take effect on config values? also, not sure why you would have a .env in your project w/o it being related. Not sure

Maybe it's possible to write a Figment provider? Although that would mean things outside the scope of config won't be interpreted (NO_COLOR etc)

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also rewrites the Solc reporter using yansi so the indicator respects NO_COLOR. Unfortunately it does not seem like yansi has the "clear line" etc. escape sequences

can split this to another PR?

Comment on lines +193 to +194
/// We could use `tracing::warn!` here, but that would imply that the dotenv file can't configure
/// the logging behavior of Foundry.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will now auto load regardless which is a big change.

I think we should consider making this opt-in via Config?

cli/src/term.rs Outdated Show resolved Hide resolved
@onbjerg
Copy link
Member Author

onbjerg commented Aug 3, 2022

can split this to another PR?

yep

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be always on? i'd make it opt-in via foundry toml maybe? dotenv = "<path>"?

@onbjerg
Copy link
Member Author

onbjerg commented Aug 3, 2022

@gakonst I don't see why you would have a .env in your project directory if it's unrelated. We can make it configurable but I'm not entirely sure how it will work with config - if we load config first, and then load .env, then .env won't have any effect on the config values unless we reload the config again I think? Don't feel strongly either way if feasible, but I think people do expect it to be always on

Edit: Alternatively, if Clap allows, we could have a --env <path> global flag, but I don't know if that's satisfying for people who want it autoloaded :S

@gakonst
Copy link
Member

gakonst commented Aug 3, 2022

ok let's just ship it as-is and see what the feedback is :p

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check for environment variables in .env files
3 participants