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 multiline support #3

Merged
merged 4 commits into from
Aug 22, 2022
Merged

Conversation

hoijui
Copy link
Contributor

@hoijui hoijui commented Mar 7, 2022

A small improvement over PR dotenv-rs/dotenv#63, as suggested in a comment there.

Fixes issue dotenv-rs/dotenv#40

@pksunkara
Copy link

KEY=asd'f => ERROR
KEY=asd'f' => asdf
KEY=asd'f'" => ERROR
KEY=asd'f'"a" => asdfa
KEY=asd'f"' => asdf"
KEY=1'2'3' => ERROR
KEY=1'2'3'4' => 1234

I think we should only remove quotes when they are at the beginning and end.

KEY=asd'f => asd'f
KEY=asd'f' => asd'f'
KEY=asd'f'" => asd'f'"
KEY=asd'f'"a" => asd'f'"a"
KEY=asd'f"' => asd'f"'
KEY=1'2'3' => 1'2'3'
KEY=1'2'3'4' => 1'2'3'4'

@hoijui
Copy link
Contributor Author

hoijui commented Mar 7, 2022

@pksunkara
That would be cleaner in many ways, but it is not how (BA)SH does it, and as I get it, .env files are kind of (BA)SH code.
So in general, we have to support that. I could see merit to support your way optionally though, when used as a library for parsing similar files.

@pksunkara
Copy link

We need to look into how other dotenv libraries support them. Those are what we need to be consistent with, not bash IIUC.

@hoijui
Copy link
Contributor Author

hoijui commented Mar 8, 2022

puh... I am not sure about that, but I certainly want to have BASH compatible mode at least as an option.

With a short search, I found the main following dotenv libraries (next to this one),
and I shortly evaluated them looking at the unit tests for parsing:

  • Node.js -
    Does not support BASH style "combined-quoting regions"
  • Python -
    Does not support BASH style "combined-quoting regions", but does support other BASH style things,
    like export key=value, or "key"='value'.

They both also support back-tics as a third valid quoting style, which again is not BASH compatible, as back-tics have special meaning there.

In short... it's complicated. ;-)
A modular way of having different parsing styles, chooseable at runtime, would be great.

@allan2
Copy link
Owner

allan2 commented Mar 8, 2022

Wow, first PR! This sounds like it warrants some more discussion so I will hold off on merging for now. Thanks @hoijui for this!

@hoijui
Copy link
Contributor Author

hoijui commented Mar 8, 2022

I don't think the compatibility issue warrants not merging this PR.
Right now, this library supports less then the other dotenv libs. With this PR, it supports more, and we might want to cut back on that. but the state with these changes is better then without.

@hoijui
Copy link
Contributor Author

hoijui commented Mar 9, 2022

I also think it would be bad, to use an BASH inspired software but not be BASH compatible as much as possible. Bash is, in my eyes, also a dotenv library, and it is likely the most used one. This code gives us quite good BASH compatibility, and to not keep it at least as an option, makes no sense to me.
To have it as an option, and to add back-tic support, would both be building upon this code (or code like it). So This stuff all should be discussed, but I see no sense in doing it here.

@pksunkara
Copy link

My question is, why would we need to be bash compatible? Are there scenarios where we load the env files directly in bash? And if we are talking about bash, why are we not looking at other shells like zsh?

@hoijui
Copy link
Contributor Author

hoijui commented Mar 12, 2022

Are there scenarios where we load the env files directly in bash?

A very clear: yes

And if we are talking about bash, why are we not looking at other shells like zsh?

BASH is surely the most important, because most widely used shell, yet looking at other shells in addition to it might also make sense, at least if it does not introduce incompatibilities.

What - if not shell compatibility - is the benefit of env files over something like JSON, YAML, TOML, XML, ...?

@pksunkara
Copy link

pksunkara commented Mar 12, 2022

A very clear: yes

What - if not shell compatibility - is the benefit of env files over something like JSON, YAML, TOMLM, XML, ...?

You are probably right, but I would still like an example scenario explained because I have never done it personally because all these other libraries are not bash compatible

@RustPanda
Copy link

RustPanda commented Apr 18, 2022

Hello. I noticed that your fork supports the multi-line environment. It's very convenient.
I checked it works:

test.sh

export MULTILINE="{
    'key': 'value'
}"

$ source ./test.sh

$ echo $MULTILINE

{
    'key': 'value'
}

i am sure this will help the rust community

@RustPanda
Copy link

RustPanda commented Apr 18, 2022

.env:

export MULTILINE='{
    "key": "value"
}'

Cargo.toml:

[package]
name = "dotenv-test"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
dotenv = {version = "0.15", git = "https://github.com/hoijui/dotenv.git"}
serde_json = "1"
clap = {version = "3", features = ["env", "derive"]}
serde = { version = "1.0", features = ["derive"] }

main.rs:

use std::str::FromStr;

use clap::Parser;
use serde::Deserialize;

#[derive(Debug, Parser)]
struct Args {
    #[clap(long, env)]
    multiline: Test,
}

#[derive(Debug, Deserialize)]
struct Test {
    #[serde(alias = "key")]
    _key: String,
}

impl FromStr for Test {
    type Err = serde_json::Error;
    fn from_str(s: &str) -> Result<Self, Self::Err> {
        serde_json::de::from_str(s)
    }
}

fn main() {
    dotenv::dotenv().unwrap();
    let args = Args::parse();

    let test = args.multiline;

    println!("{test:#?}");
}

Output:

➜  donenv-test git:(main) ✗ cargo run 
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/donenv-test`
Test {
    _key: "value",
}

@onx2
Copy link

onx2 commented Aug 22, 2022

Any chance this could get merged in if all checks are good? It would be very helpful for certs and keys 😄 Thanks for the contribution!

@allan2 allan2 merged commit 1cc68b5 into allan2:master Aug 22, 2022
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.

6 participants